Skip to content

Commit

Permalink
fix: ledger signature recovery value (#408)
Browse files Browse the repository at this point in the history 
<!-- start pr-codex -->

## PR-Codex overview
This PR enhances support for `celo-legacy` and modern transactions
within the `ledger` wallet, including updates to transaction signing,
error handling, and test cases.

### Detailed summary
- Updated `package.json` for `@ledgerhq/hw-app-eth` to a specific
commit.
- Added `bundleDependencies` for `@ledgerhq/hw-app-eth`.
- Improved `signTransaction` method in `LedgerSigner` to handle `v`
values correctly.
- Refactored transaction recovery logic in tests for better validation.
- Enhanced error messages for legacy app version requirements.
- Updated tests to validate transaction signing for both legacy and
modern types.

> ✨ Ask PR-Codex anything about this PR by commenting with `/codex {your
question}`

<!-- end pr-codex -->
  • Loading branch information
@nicolasbrugneaux
nicolasbrugneaux authored Nov 12, 2024
1 parent 76045eb commit d988d31
Show file tree
Hide file tree
Showing 13 changed files with 579 additions and 267 deletions.
7 changes: 7 additions & 0 deletions .changeset/little-carpets-argue.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
'@celo/wallet-ledger': patch
'@celo/wallet-local': patch
'@celo/wallet-base': patch
---

Improve support for celo-legacy and modern txs within ledger
8 changes: 5 additions & 3 deletions .github/workflows/cron-npm-install.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,18 @@ jobs:
fail-fast: false
matrix:
package:
- '@celo/utils@beta'
- '@celo/contractkit@beta'
- '@celo/celocli@beta'
- '@celo/utils@beta'
- '@celo/contractkit@beta'
- '@celo/celocli@beta'
steps:
- name: Install @celo/celocli dependencies
if: matrix.package == '@celo/celocli@beta'
run: |
apt update
apt install -y libusb-1.0-0-dev libudev-dev
npm install node-gyp --global
git config --global url."https://".insteadOf ssh://
- name: Installing npm package
run: npm install ${{ matrix.package }} --global
- name: Test celocli command
Expand Down
6 changes: 3 additions & 3 deletions docs/sdk/wallet-base/interfaces/signing_utils.LegacyEncodedTx.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@

#### Defined in

[wallets/wallet-base/src/signing-utils.ts:232](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L232)
[wallets/wallet-base/src/signing-utils.ts:240](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L240)

___

Expand All @@ -30,7 +30,7 @@ ___

#### Defined in

[wallets/wallet-base/src/signing-utils.ts:233](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L233)
[wallets/wallet-base/src/signing-utils.ts:241](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L241)

___

Expand All @@ -40,4 +40,4 @@ ___

#### Defined in

[wallets/wallet-base/src/signing-utils.ts:231](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L231)
[wallets/wallet-base/src/signing-utils.ts:239](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L239)
48 changes: 24 additions & 24 deletions docs/sdk/wallet-base/modules/signing_utils.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ ___

#### Defined in

[wallets/wallet-base/src/signing-utils.ts:935](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L935)
[wallets/wallet-base/src/signing-utils.ts:946](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L946)

___

Expand Down Expand Up @@ -139,7 +139,7 @@ ___

#### Defined in

[wallets/wallet-base/src/signing-utils.ts:869](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L869)
[wallets/wallet-base/src/signing-utils.ts:880](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L880)

___

Expand All @@ -159,7 +159,7 @@ ___

#### Defined in

[wallets/wallet-base/src/signing-utils.ts:545](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L545)
[wallets/wallet-base/src/signing-utils.ts:553](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L553)

___

Expand All @@ -183,7 +183,7 @@ ___

#### Defined in

[wallets/wallet-base/src/signing-utils.ts:368](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L368)
[wallets/wallet-base/src/signing-utils.ts:376](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L376)

___

Expand All @@ -203,7 +203,7 @@ ___

#### Defined in

[wallets/wallet-base/src/signing-utils.ts:237](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L237)
[wallets/wallet-base/src/signing-utils.ts:245](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L245)

___

Expand All @@ -229,7 +229,7 @@ ___

#### Defined in

[wallets/wallet-base/src/signing-utils.ts:879](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L879)
[wallets/wallet-base/src/signing-utils.ts:890](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L890)

___

Expand All @@ -255,7 +255,7 @@ ___

#### Defined in

[wallets/wallet-base/src/signing-utils.ts:463](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L463)
[wallets/wallet-base/src/signing-utils.ts:471](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L471)

___

Expand Down Expand Up @@ -295,7 +295,7 @@ ___

#### Defined in

[wallets/wallet-base/src/signing-utils.ts:535](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L535)
[wallets/wallet-base/src/signing-utils.ts:543](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L543)

___

Expand All @@ -315,7 +315,7 @@ ___

#### Defined in

[wallets/wallet-base/src/signing-utils.ts:913](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L913)
[wallets/wallet-base/src/signing-utils.ts:924](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L924)

___

Expand All @@ -335,7 +335,7 @@ ___

#### Defined in

[wallets/wallet-base/src/signing-utils.ts:927](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L927)
[wallets/wallet-base/src/signing-utils.ts:938](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L938)

___

Expand All @@ -355,7 +355,7 @@ ___

#### Defined in

[wallets/wallet-base/src/signing-utils.ts:919](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L919)
[wallets/wallet-base/src/signing-utils.ts:930](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L930)

___

Expand All @@ -375,7 +375,7 @@ ___

#### Defined in

[wallets/wallet-base/src/signing-utils.ts:901](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L901)
[wallets/wallet-base/src/signing-utils.ts:912](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L912)

___

Expand All @@ -395,7 +395,7 @@ ___

#### Defined in

[wallets/wallet-base/src/signing-utils.ts:907](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L907)
[wallets/wallet-base/src/signing-utils.ts:918](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L918)

___

Expand All @@ -415,7 +415,7 @@ ___

#### Defined in

[wallets/wallet-base/src/signing-utils.ts:347](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L347)
[wallets/wallet-base/src/signing-utils.ts:355](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L355)

___

Expand All @@ -435,7 +435,7 @@ ___

#### Defined in

[wallets/wallet-base/src/signing-utils.ts:343](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L343)
[wallets/wallet-base/src/signing-utils.ts:351](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L351)

___

Expand All @@ -455,7 +455,7 @@ ___

#### Defined in

[wallets/wallet-base/src/signing-utils.ts:339](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L339)
[wallets/wallet-base/src/signing-utils.ts:347](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L347)

___

Expand All @@ -475,7 +475,7 @@ ___

#### Defined in

[wallets/wallet-base/src/signing-utils.ts:325](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L325)
[wallets/wallet-base/src/signing-utils.ts:333](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L333)

___

Expand All @@ -496,7 +496,7 @@ ___

#### Defined in

[wallets/wallet-base/src/signing-utils.ts:837](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L837)
[wallets/wallet-base/src/signing-utils.ts:848](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L848)

___

Expand All @@ -516,7 +516,7 @@ ___

#### Defined in

[wallets/wallet-base/src/signing-utils.ts:494](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L494)
[wallets/wallet-base/src/signing-utils.ts:502](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L502)

___

Expand All @@ -536,7 +536,7 @@ ___

#### Defined in

[wallets/wallet-base/src/signing-utils.ts:126](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L126)
[wallets/wallet-base/src/signing-utils.ts:134](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L134)

___

Expand Down Expand Up @@ -564,7 +564,7 @@ ___

#### Defined in

[wallets/wallet-base/src/signing-utils.ts:892](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L892)
[wallets/wallet-base/src/signing-utils.ts:903](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L903)

___

Expand All @@ -584,7 +584,7 @@ ___

#### Defined in

[wallets/wallet-base/src/signing-utils.ts:107](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L107)
[wallets/wallet-base/src/signing-utils.ts:115](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L115)

___

Expand All @@ -606,7 +606,7 @@ ___

#### Defined in

[wallets/wallet-base/src/signing-utils.ts:847](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L847)
[wallets/wallet-base/src/signing-utils.ts:858](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L858)

___

Expand All @@ -628,4 +628,4 @@ ___

#### Defined in

[wallets/wallet-base/src/signing-utils.ts:856](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L856)
[wallets/wallet-base/src/signing-utils.ts:867](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L867)
12 changes: 6 additions & 6 deletions docs/sdk/wallet-ledger/classes/ledger_signer.LedgerSigner.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ Signer.computeSharedSecret

#### Defined in

[wallet-ledger/src/ledger-signer.ts:197](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-ledger/src/ledger-signer.ts#L197)
[wallet-ledger/src/ledger-signer.ts:190](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-ledger/src/ledger-signer.ts#L190)

___

Expand All @@ -96,7 +96,7 @@ Signer.decrypt

#### Defined in

[wallet-ledger/src/ledger-signer.ts:191](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-ledger/src/ledger-signer.ts#L191)
[wallet-ledger/src/ledger-signer.ts:184](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-ledger/src/ledger-signer.ts#L184)

___

Expand Down Expand Up @@ -138,19 +138,19 @@ Signer.signPersonalMessage

#### Defined in

[wallet-ledger/src/ledger-signer.ts:86](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-ledger/src/ledger-signer.ts#L86)
[wallet-ledger/src/ledger-signer.ts:79](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-ledger/src/ledger-signer.ts#L79)

___

### signTransaction

▸ **signTransaction**(`addToV`, `encodedTx`): `Promise`\<\{ `r`: `Buffer` ; `s`: `Buffer` ; `v`: `number` }\>
▸ **signTransaction**(`_addToV`, `encodedTx`): `Promise`\<\{ `r`: `Buffer` ; `s`: `Buffer` ; `v`: `number` }\>

#### Parameters

| Name | Type |
| :------ | :------ |
| `addToV` | `number` |
| `_addToV` | `number` |
| `encodedTx` | `RLPEncodedTx` \| `LegacyEncodedTx` |

#### Returns
Expand Down Expand Up @@ -187,4 +187,4 @@ Signer.signTypedData

#### Defined in

[wallet-ledger/src/ledger-signer.ts:106](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-ledger/src/ledger-signer.ts#L106)
[wallet-ledger/src/ledger-signer.ts:99](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/wallets/wallet-ledger/src/ledger-signer.ts#L99)
55 changes: 52 additions & 3 deletions packages/sdk/wallets/wallet-base/src/signing-utils.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -545,15 +545,15 @@ describe('recoverTransaction', () => {
"s": "0x1799477e0f601f554f0ee6f7ac21490602124801e9f7a99d9605249b90f03112",
"to": "0x588e4b68193001e4d10928660ab4165b813717c0",
"type": "cip42",
"v": 28,
"v": 27,
"value": 1000000000000000000,
"yParity": 1,
"yParity": 0,
},
"0x90AB065B949165c47Acac34cA9A43171bBeBb1E1",
]
`)
})
test('cip42 serialized by viem', async () => {
test('cip42 serialized by viem (v=0)', async () => {
const account = privateKeyToAccount(PRIVATE_KEY1)
const signed = await account.signTransaction(
{
Expand Down Expand Up @@ -601,6 +601,55 @@ describe('recoverTransaction', () => {
`)
expect(recoverTransaction(signed)[1]).toEqual(account.address)
})

test.only('cip42 serialized by viem (v=1)', async () => {
const account = privateKeyToAccount(PRIVATE_KEY1)
const signed = await account.signTransaction(
{
// @ts-ignore -- types on viem dont quite work for setting the tx type but the actual js execution works fine
type: 'cip42',
accessList: [],
chainId: 44378,
data: '0xabcdef',
feeCurrency: '0xcd2a3d9f938e13cd947ec05abc7fe734df8dd826',
gas: BigInt(10),
gatewayFee: BigInt('0x5678'),
gatewayFeeRecipient: '0x1be31a94361a391bbafb2a4ccd704f57dc04d4bb',
maxFeePerGas: BigInt(99),
maxPriorityFeePerGas: BigInt(99),
nonce: 2,
to: '0x588e4b68193001e4d10928660ab4165b813717c0',
value: BigInt(1000000000000000000),
},
{ serializer: celo.serializers?.transaction }
)

expect(recoverTransaction(signed)).toMatchInlineSnapshot(`
[
{
"accessList": [],
"chainId": 44378,
"data": "0xabcdef",
"feeCurrency": "0xcd2a3d9f938e13cd947ec05abc7fe734df8dd826",
"gas": 10,
"gatewayFee": "0x5678",
"gatewayFeeRecipient": "0x1be31a94361a391bbafb2a4ccd704f57dc04d4bb",
"maxFeePerGas": 99,
"maxPriorityFeePerGas": 99,
"nonce": 2,
"r": "0xd8ac17c9add66e5406edcbd925c0fe0257758467c0f1e85c412665f43e472ed6",
"s": "0x7235dcebf2a7082298069dc75a2a8049d9929dbac528e93ad6611047fc7fedc9",
"to": "0x588e4b68193001e4d10928660ab4165b813717c0",
"type": "cip42",
"v": 28,
"value": 1000000000000000000,
"yParity": 1,
},
"0x1Be31A94361a391bBaFB2a4CCd704F57dc04d4bb",
]
`)
expect(recoverTransaction(signed)[1]).toEqual(account.address)
})
})

describe('isPriceToLow', () => {
Expand Down
Loading

0 comments on commit d988d31

Please sign in to comment.