Update zondax ironfish to 0.5.1 #5449
Conversation
We previously had to have two versions of this SDK available because the newer one did not work with single signer. Zondax fixed the bugs and now we don't need to manage two separate dependencies now. Created a base ledger class for the common functionality.
patnir
force-pushed
the
rahul/update-zondax-js-to-5-1
branch
from
a67544c to
a250002
Compare
| ResponseIdentity, | ||
| ResponseProofGenKey as ResponseProofGenKeyDkg, | ||
| ResponseViewKey as ResponseViewKeyDkg, | ||
| } from '@zondax/ledger-ironfish-dkg' | ||
| import { ResponseError } from '@zondax/ledger-js' |
There was a problem hiding this comment.
This is a bug because we're importing from a library that we don't have a dependency on. We shouldn't import from dependencies of our dependencies. We need to include this in our package.json.
| @@ -68,9 +68,7 @@ | |||
| "@oclif/plugin-warn-if-update-available": "3.1.8", | |||
| "@types/keccak": "3.0.4", | |||
| "@types/tar": "6.1.1", | |||
| "@zondax/ledger-ironfish": "0.1.2", | |||
| "@zondax/ledger-ironfish-dkg": "npm:@zondax/ledger-ironfish@0.4.0", | |||
| "@zondax/ledger-js": "^1.0.1", | |||
There was a problem hiding this comment.
I asked Zondax if they will the response error type. I feel like we shouldn't include ledger-js since it is an inner dependency. We only had this because we were managing two versions of ledger-ironfish which relied on different versions of this sub dependency
There was a problem hiding this comment.
We added this dependency because we needed access to ResponseError. Managing two versions of @zondax/ledger-ironfish caused problems with resolving to the right version, but we needed our own dependency for ResponseError
There was a problem hiding this comment.
I think that's fine, but you cannot use an import without referencing it in your package because if the library you depend on stops including it your code will stop compiling. You are making the code base depend on that library by importing it, therefore you need to add it to the package to indicate the dependency.
There was a problem hiding this comment.
A good way to see this in action is to comment out import { ResponseError } from '@zondax/ledger-js' in ledger.ts, then press CMD + . in VSCode to trigger the autofix. Without this being in the package.json, it will not attempt to auto-import anything. If this is in the package.json, it correctly suggests to add the import.
There was a problem hiding this comment.
@mat-if tried this, it is not in this package.json . They haven't exposed it in "@zondax/ledger-ironfish". I asked zondax to do it, and they said they should release it today
|
Going to merge this now and made a note to import ResponseError from the top level dependency once zondax makes the change. Context: https://iflabs.slack.com/archives/C06FQDY6CNT/p1727824902604019 |
Summary
We previously had to have two versions of this SDK available because the newer one did not work with single signer.
Zondax fixed the bugs and now we don't need to manage two separate dependencies now.
Created a base ledger class for the common functionality.
Testing Plan
Documentation
Does this change require any updates to the Iron Fish Docs (ex. the RPC API
Reference)? If yes, link a
related documentation pull request for the website.
Breaking Change
Is this a breaking change? If yes, add notes below on why this is breaking and label it with
breaking-change-rpcorbreaking-change-sdk.