From 634fbfa2d4a0d5f801828a05dc6a1117a0c15ee4 Mon Sep 17 00:00:00 2001 From: Aaron DeRuvo Date: Mon, 14 Oct 2024 13:23:44 +0200 Subject: [PATCH] Fixing Ledger 1.1.10 interacting with whitelisted fee token contracts (#395) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There was a bug where ledgers would return a INVALID DATA error when trying to transfer or interactive with a contract of one of the whitelisted gas fee tokens. or using a gas token to pay for gas. This occured while checking if the token was known in order to display human readble data while confirming on the ledger. The legacy 1.1.10 version of ledger app uses an older blob of signed data. it seems in this case the encoded data should NOT be prefixed by 0x. while in the newer it should be. this was not obvious and as automated tests are difficult when interacting with hw devices was missed. turned off test for cip66. Possibly we will remove later. fixes #394 fixes #353 --- This PR focuses on fixing issues related to token transfers and gas currency handling in the `celo` ecosystem, particularly with the `@celo/celocli` and `@celo/wallet-ledger` packages. It also improves error messages and adds a new method for providing token information. - Updated error messages in `packages/cli/src/transfer-stable-base.ts` for clarity on transfer affordability. - Fixed token information handling in `packages/sdk/wallets/wallet-ledger/src/ledger-signer.ts`. - Added `provideERC20TokenInformation` method to `LedgerSigner`. - Improved tests in `packages/sdk/wallets/wallet-ledger/src/ledger-wallet.test.ts` with mock implementations and inline snapshots. - Updated test suite to skip certain tests related to `cip66`. > ✨ Ask PR-Codex anything about this PR by commenting with `/codex {your question}` --- .changeset/fair-points-beg.md | 5 +++ .changeset/hungry-cups-juggle.md | 5 +++ .changeset/many-cobras-live.md | 5 +++ packages/cli/src/transfer-stable-base.ts | 6 ++-- .../wallet-ledger/src/ledger-signer.ts | 17 ++++++++-- .../wallet-ledger/src/ledger-wallet.test.ts | 32 +++++++++++++++++-- 6 files changed, 64 insertions(+), 6 deletions(-) create mode 100644 .changeset/fair-points-beg.md create mode 100644 .changeset/hungry-cups-juggle.md create mode 100644 .changeset/many-cobras-live.md diff --git a/.changeset/fair-points-beg.md b/.changeset/fair-points-beg.md new file mode 100644 index 000000000..b55157c54 --- /dev/null +++ b/.changeset/fair-points-beg.md @@ -0,0 +1,5 @@ +--- +'@celo/wallet-ledger': patch +--- + +Fix issue where ledger running celo firmware app 1.1.10 could not send fee token transactions or perform and interactions with those contracts diff --git a/.changeset/hungry-cups-juggle.md b/.changeset/hungry-cups-juggle.md new file mode 100644 index 000000000..7ea711cda --- /dev/null +++ b/.changeset/hungry-cups-juggle.md @@ -0,0 +1,5 @@ +--- +'@celo/celocli': patch +--- + +Fix incorrect message where the transfered token was used as gas token in the messaging but not in actuality diff --git a/.changeset/many-cobras-live.md b/.changeset/many-cobras-live.md new file mode 100644 index 000000000..4f6b73042 --- /dev/null +++ b/.changeset/many-cobras-live.md @@ -0,0 +1,5 @@ +--- +'@celo/celocli': patch +--- + +Fix Transfering, exchanging cusd (and other fee tokens) and or using gasCurrency flag with ledger devices prior to 1.2 diff --git a/packages/cli/src/transfer-stable-base.ts b/packages/cli/src/transfer-stable-base.ts index ace0ce0da..7002e6e38 100644 --- a/packages/cli/src/transfer-stable-base.ts +++ b/packages/cli/src/transfer-stable-base.ts @@ -79,7 +79,7 @@ export abstract class TransferStableBase extends BaseCommand { .isNotSanctioned(from) .isNotSanctioned(to) .addCheck( - `Account can afford transfer in ${this._stableCurrency} and gas paid in ${ + `Account can afford to transfer ${this._stableCurrency} and gas paid in ${ res.flags.gasCurrency || 'CELO' }`, async () => { @@ -103,7 +103,9 @@ export abstract class TransferStableBase extends BaseCommand { } return valueBalance.gte(value.plus(gasValue)) }, - `Cannot afford transfer with ${this._stableCurrency} gasCurrency; try reducing value slightly or using gasCurrency=CELO` + `Cannot afford to transfer ${this._stableCurrency} ${ + res.flags.gasCurrency ? 'with' + ' ' + res.flags.gasCurrency + ' ' + 'gasCurrency' : '' + }; try reducing value slightly or using a different gasCurrency` ) .runChecks() diff --git a/packages/sdk/wallets/wallet-ledger/src/ledger-signer.ts b/packages/sdk/wallets/wallet-ledger/src/ledger-signer.ts index e656b4764..40e7f2164 100644 --- a/packages/sdk/wallets/wallet-ledger/src/ledger-signer.ts +++ b/packages/sdk/wallets/wallet-ledger/src/ledger-signer.ts @@ -174,7 +174,7 @@ export class LedgerSigner implements Signer { const tokenInfo = getTokenInfo(rlpEncoded.transaction.to!, rlpEncoded.transaction.chainId!) if (tokenInfo) { - await this.ledger!.provideERC20TokenInformation(`0x${tokenInfo.data.toString('hex')}`) + await this.provideERC20TokenInformation(tokenInfo.data) } if (rlpEncoded.transaction.feeCurrency && rlpEncoded.transaction.feeCurrency !== '0x') { const feeTokenInfo = getTokenInfo( @@ -182,7 +182,7 @@ export class LedgerSigner implements Signer { rlpEncoded.transaction.chainId! ) if (feeTokenInfo) { - await this.ledger!.provideERC20TokenInformation(feeTokenInfo.data.toString('hex')) + await this.provideERC20TokenInformation(feeTokenInfo.data) } } } @@ -198,4 +198,17 @@ export class LedgerSigner implements Signer { throw new Error('Not implemented') return Promise.resolve(Buffer.from([])) } + + private provideERC20TokenInformation(tokenInfoData: Buffer) { + // it looks like legacy might need it WITHOUT 0x prefix + const isModern = meetsVersionRequirements(this.appConfiguration.version, { + minimum: LedgerWallet.MIN_VERSION_EIP1559, + }) + + const hexStringTokenInfo = isModern + ? ensureLeading0x(tokenInfoData.toString('hex')) + : trimLeading0x(tokenInfoData.toString('hex')) + + return this.ledger!.provideERC20TokenInformation(hexStringTokenInfo) + } } diff --git a/packages/sdk/wallets/wallet-ledger/src/ledger-wallet.test.ts b/packages/sdk/wallets/wallet-ledger/src/ledger-wallet.test.ts index b70b60c83..cf3bef77f 100644 --- a/packages/sdk/wallets/wallet-ledger/src/ledger-wallet.test.ts +++ b/packages/sdk/wallets/wallet-ledger/src/ledger-wallet.test.ts @@ -700,12 +700,40 @@ describe('LedgerWallet class', () => { test( 'succeeds', async () => { - await expect(wallet.signTransaction(celoTransaction)).resolves.not.toBeUndefined() + jest + .spyOn(wallet.ledger!, 'provideERC20TokenInformation') + .mockImplementationOnce(async () => true) + await expect(wallet.signTransaction(celoTransaction)).resolves + .toMatchInlineSnapshot(` + { + "raw": "0x7bf87f82aef38063636394588e4b68193001e4d10928660ab4165b813717c0880de0b6b3a764000080c094874069fa1eb16d44d622f2e0ca25eea172369bc101a0254f952c5223c30039f7f845778d7aac558464ce2971fd09883df34913eb6dfca037a78571ae1a44d86bac7269e3a845990a49ad5fb60a5ec1fcaba428693558c0", + "tx": { + "accessList": [], + "feeCurrency": "0x874069fa1eb16d44d622f2e0ca25eea172369bc1", + "gas": "0x63", + "hash": "0xdc8347423b5310ed64e46a9abb49cd455e8049f838f93752afd122ae938e53c9", + "input": "0x", + "maxFeePerGas": "0x63", + "maxPriorityFeePerGas": "0x63", + "nonce": "0", + "r": "0x254f952c5223c30039f7f845778d7aac558464ce2971fd09883df34913eb6dfc", + "s": "0x37a78571ae1a44d86bac7269e3a845990a49ad5fb60a5ec1fcaba428693558c0", + "to": "0x588e4b68193001e4d10928660ab4165b813717c0", + "v": "0x01", + "value": "0x0de0b6b3a7640000", + }, + "type": "cip64", + } + `) + + expect(wallet.ledger!.provideERC20TokenInformation).toHaveBeenCalledWith( + `0x06612063555344874069fa1eb16d44d622f2e0ca25eea172369bc1000000120000aef33045022100a885480c357fd6ec64ed532656a7e988198fdf4e2cf4632408f2d65561189872022009fd78725055fc68af16e151516ba29625e3e1c74ceab3da1bcabd6015e3f6e8` + ) }, TEST_TIMEOUT_IN_MS ) }) - describe('[cip66]', () => { + describe.skip('[cip66]', () => { const kit = newKit('https://alfajores-forno.celo-testnet.org') beforeEach(async () => { celoTransaction = {