Skip to content

Commit

Permalink
Fixing Ledger 1.1.10 interacting with whitelisted fee token contracts (
Browse files Browse the repository at this point in the history
…#395)

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

<!-- start pr-codex -->

---

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}`

<!-- end pr-codex -->
  • Loading branch information
aaronmgdr committed Oct 14, 2024
1 parent ee8c2ba commit 634fbfa
Show file tree
Hide file tree
Showing 6 changed files with 64 additions and 6 deletions.
5 changes: 5 additions & 0 deletions .changeset/fair-points-beg.md
Original file line number Diff line number Diff line change
@@ -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
5 changes: 5 additions & 0 deletions .changeset/hungry-cups-juggle.md
Original file line number Diff line number Diff line change
@@ -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
5 changes: 5 additions & 0 deletions .changeset/many-cobras-live.md
Original file line number Diff line number Diff line change
@@ -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
6 changes: 4 additions & 2 deletions packages/cli/src/transfer-stable-base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand All @@ -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()

Expand Down
17 changes: 15 additions & 2 deletions packages/sdk/wallets/wallet-ledger/src/ledger-signer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -174,15 +174,15 @@ 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(
rlpEncoded.transaction.feeCurrency!,
rlpEncoded.transaction.chainId!
)
if (feeTokenInfo) {
await this.ledger!.provideERC20TokenInformation(feeTokenInfo.data.toString('hex'))
await this.provideERC20TokenInformation(feeTokenInfo.data)
}
}
}
Expand All @@ -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)
}
}
32 changes: 30 additions & 2 deletions packages/sdk/wallets/wallet-ledger/src/ledger-wallet.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down

0 comments on commit 634fbfa

Please sign in to comment.