feat(ledger): map Unichain, so its deployments are no longer skipped - #346
Open
venugopalanvip wants to merge 1 commit into
Open
venugopalanvip wants to merge 1 commit into
venugopalanvip wants to merge 1 commit into
Conversation
`ledger_network_id` returns None for chain 130, so the calldata converter drops
every deployment on it with a warning and emits no device descriptor for that
chain. Ledger does support it: `@ledgerhq/cryptoassets@13.56.0` carries
unichain { type: CryptoCurrency, family: evm, chainId: 130 }
unichain_sepolia { type: CryptoCurrency, family: evm, chainId: 1301 }
Both ids are taken verbatim from that package rather than guessed, and every one
of the 61 chain ids this file shares with it already agrees exactly.
Seven descriptors in the clear-signing registry declare a deployment on one of
the two, and lose it today -- among them Lido's L2 stETH and wstETH, OKX's
router on both versions, and Kyberswap's aggregator.
The registry uses 95 distinct chain ids across its calldata descriptors, of
which this file knew 48. The other 46 are not in the cryptoassets package
either, so they have no Ledger currency id to map to and are left alone.
Tests pin the two new ids, the None for an unsupported chain, and the ascending
order of the match arms, which is what makes the insertion point obvious.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The gap
ledger_network_idreturnsNonefor chain 130, soconvert_erc7730_v2_input_to_calldatadrops every deployment on it:No device descriptor is produced for that chain, and the only signal is a warning on a converter run that still exits successfully.
Ledger does support Unichain.
@ledgerhq/cryptoassets@13.56.0carries both:Both ids are taken verbatim from that package rather than guessed. As a check on the source: of the 123 chain ids this file already maps, 61 also appear in the package, and all 61 agree exactly — so it is the right list, and the ids are in the right form.
What it recovers
Seven descriptors in the clear-signing registry declare a deployment on one of the two and lose it today:
registry/ekubo/calldata-MEVCaptureRouter.jsonregistry/kyberswap/calldata-MetaAggregationRouterV2.jsonregistry/layerswap/calldata-LayerswapDepository.jsonregistry/lido/calldata-stETH-L2.jsonregistry/lido/calldata-wstETH-L2.jsonregistry/okx/calldata-OkxDexRouterV1.0.7-multi-commission.jsonregistry/okx/calldata-OkxDexRouterV1.0.8-suffix-compat.jsonlido/calldata-stETH-L2.jsonis the sharpest: it declares two deployments, and one of them is Unichain.The wider measurement, and why this PR stops here
I swept every calldata descriptor in the registry to find how much this costs in total:
ledger_network_idSo roughly half the chains the registry targets produce no device output. But of those 47, only these two are in the cryptoassets package. The other 45 — Plasma (9745, 12 deployments), Soneium (1868), TAC (239), MegaETH (4326), Manta Pacific (169), opBNB, Fraxtal, Ronin, Taiko, Zora and the rest — have no Ledger currency id at all, so there is nothing correct to map them to. Dropping them is the right behaviour, not a bug, and inventing ids for them would be worse than the warning.
That is why this PR adds two lines rather than forty-seven. If Ledger adds currencies for any of those chains, the same one-line-per-chain change applies; happy to file the full list as an issue if it is useful for tracking.
Tests
tests/common/test_ledger_network_id.pyis new — the file had none. It pins the two new ids, an existing one either side, theNonefor an unsupported chain, and that the match arms stay in ascending chain-id order, which is what makes the insertion point for the next chain unambiguous.