fix: revert ledger clearSignTransaction back to signTransaction. #41899
fix: revert ledger clearSignTransaction back to signTransaction. #41899montelaidev wants to merge 4 commits into
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
✨ Files requiring CODEOWNER review ✨🔑 @MetaMask/accounts-engineers (2 files, +9 -277)
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1b59543. Configure here.
Builds ready [0c8eabf]
⚡ Performance Benchmarks (Total: 🟢 7 pass · 🟡 8 warn · 🔴 0 fail)
Bundle size diffs [🚀 Bundle size reduced!]
|
|
Builds ready [587c616]
⚡ Performance Benchmarks (Total: 🟢 7 pass · 🟡 8 warn · 🔴 0 fail)
Bundle size diffs [🚀 Bundle size reduced!]
|
| erc20: isERC20Tx, | ||
| nft: isNftTx, | ||
| }); | ||
| const result = await app.signTransaction(hdPath, tx, null); |
There was a problem hiding this comment.
Sorry, I saw your change directly to use signTransaction() which means ledger clear signing has completely disabled for all network which is not right in my opionion. previously Ledger team suggest us to just use try catch block to try clear signing first and then fallback to use transitional signing if clear sign not supported.
So i think using try catch block to try clear sign fist and then fall back to tranditional signing is better solution which means metamask can still take advantage of ledger clear signing for supported tokens and chains.
There was a problem hiding this comment.
@dawnseeker8 in my testing I did not notice any differences in what was displayed on the device between prod on this branch. That being said I did not complete all my testing and I do think NFTs might be effected. @montelaidev can confirm but I believe a try catch solution would result in the the user being prompted to sign the transaction that will fail and then be immediately prompted to sign another "Legacy" transaction.
There was a problem hiding this comment.
@montelaidev I did a little more digging and am now even more confused. The source code for clearSignTransaction already fallsback to a normal sign. If this is the case, why is this not working already then?
/**
* Helper to get resolution and signature of a transaction in a single method
*
* @param path: the BIP32 path to sign the transaction on
* @param rawTxHex: the raw ethereum transaction in hexadecimal to sign
* @param resolutionConfig: configuration about what should be clear signed in the transaction
* @param throwOnError: optional parameter to determine if a failing resolution of the transaction should throw an error or not
* @example
const tx = "e8018504e3b292008252089428ee52a8f3d6e5d15f8b131996950d7f296c7952872bd72a2487400080"; // raw tx to sign
const result = eth.clearSignTransaction("44'/60'/0'/0/0", tx, { erc20: true, externalPlugins: true, nft: true});
console.log(result);
*/
async clearSignTransaction(
path: string,
rawTxHex: string,
resolutionConfig: ResolutionConfig,
throwOnError = false,
): Promise<{ r: string; s: string; v: string }> {
const resolution = await ledgerService
.resolveTransaction(rawTxHex, this.loadConfig, resolutionConfig)
.catch(e => {
console.warn(
"an error occurred in resolveTransaction => fallback to blind signing: " + String(e),
);
if (throwOnError) {
throw e;
}
return null;
});
return this.signTransaction(path, rawTxHex, resolution);
}|
@montelaidev lets not merge this yet until we have done extensive QA and I have given the sign off. |
Description
This PR reverts the usage of clearSignTransaction back to signTransaction for ledger because of an edge case where erc20 transactions do not work on the monad network.
The root cause is that ERC20 resolution doesn't work for Monad because it's not in the list of supported networks for clear signing.
When erc20: true:
The library fetches ERC20 token signatures from Ledger's CDN — which does have 60 tokens for Monad. If the transaction targets one of those tokens via transfer or approve, a token descriptor blob (containing chainId=143) is sent to the device. The device can't process the descriptor for an unrecognized chain, enters an inconsistent state, and rejects the signing APDU with 0x6a80.
Custom networks work otherwise because without resolution data, the device blind-signs the raw transaction hash. It doesn't need to know the chain — ECDSA works on any chain. The failure only happens when ERC20/NFT/plugin resolution data is injected for an unsupported chain.
Only approve (0x095ea7b3) and transfer (0xa9059cbb) trigger the ERC20 path — all other selectors skip it.
Changelog
CHANGELOG entry: Fix issue where monad transactions do not work on ledger.
Related issues
Fixes: https://consensyssoftware.atlassian.net/browse/MUL-1466?atlOrigin=eyJpIjoiZjNjMDNmZWFiYzY5NGE2MTliOWVhM2ZlNWE0NmQyMDkiLCJwIjoiaiJ9
#41602
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Medium Risk
Changes the Ledger transaction-signing path for all networks, which can affect signing UX/compatibility and potentially what users see/confirm on-device. While the change is small, it touches hardware-wallet signing behavior and should be validated on key networks/devices.
Overview
Reverts Ledger transaction signing in the offscreen handler from
clearSignTransaction(selector-based ERC20/NFT/plugin resolution) back tosignTransaction(hdPath, tx, null), removing the selector parsing/fallback logic and related dependencies.Updates the Ledger offscreen Jest tests accordingly by dropping transaction-selector/serialization cases and asserting the simpler
signTransactioncall signature.Reviewed by Cursor Bugbot for commit 587c616. Bugbot is set up for automated code reviews on this repo. Configure here.