Fix: cip64 regressions #6

nicolasbrugneaux · 2023-10-30T18:02:58Z

This PR aims to fix the issues reported by @0xarthurxyz in celo-org/txtypes#1

This refactors the different checks for CIP42 and CIP64 by using more robust mechanism than truthy/falsy values.
It also removes the tests that added type: 'cipxyz' unnecessarily and by doing so forcing the tx to be correct even though the transaction-type inference not detect it as such. (eg: a cip42 without gatewayFee).

It also removes the brittle tests hardcoding the raw serialized string (keeps it in a couple for sanity) but uses the parser to self-check itself.

github-actions · 2023-10-30T18:08:13Z

size-limit report 📦

Path	Size	Loading time (3g)	Running time (snapdragon)	Total time
viem (esm)	58.24 KB (0%)	1.2 s (0%)	369 ms (+21.48% 🔺)	1.6 s
viem (cjs)	77.25 KB (0%)	1.6 s (0%)	888 ms (+27.77% 🔺)	2.5 s
viem (minimal surface - tree-shaking)	3.86 KB (0%)	78 ms (0%)	57 ms (-15.04% 🔽)	135 ms
viem/accounts	89.01 KB (0%)	1.8 s (0%)	399 ms (+138.5% 🔺)	2.2 s
viem/accounts (tree-shaking)	19.37 KB (0%)	388 ms (0%)	229 ms (+39.32% 🔺)	616 ms
viem/actions	43.31 KB (0%)	867 ms (0%)	2.2 s (-0.78% 🔽)	3.1 s
viem/actions (tree-shaking)	350 B (0%)	10 ms (0%)	114 ms (+200.6% 🔺)	124 ms
viem/chains	18.56 KB (0%)	372 ms (0%)	280 ms (+71.46% 🔺)	652 ms
viem/chains (tree-shaking)	470 B (0%)	10 ms (0%)	109 ms (+83.16% 🔺)	119 ms
viem/chains/utils	9.15 KB (0%)	183 ms (0%)	155 ms (+54.2% 🔺)	338 ms
viem/chains/utils (tree-shaking)	5.36 KB (0%)	108 ms (0%)	61 ms (+98.23% 🔺)	169 ms
viem/ens	43.31 KB (0%)	867 ms (0%)	1.8 s (-27.9% 🔽)	2.7 s
viem/ens (tree-shaking)	18 KB (0%)	360 ms (0%)	232 ms (+99.37% 🔺)	592 ms

src/chains/celo/formatters.test.ts

src/chains/celo/serializers.test.ts

src/chains/celo/utils.ts

arthurgousset · 2023-11-01T14:51:52Z

src/actions/wallet/signTransaction.test.ts

+  const snapshot =
+    '"0x7cf8840182031184773594008504a817c80082520894765de816845861e75a25fca122bb6898b8b1282a940f16e9b0d03470827a95cdfd0cb8a8a3b46969b904808080c001a062bee7f81cccd1f430b4b66ec5a23737d6fbee9965e63ac582d09f63aef32bdca05e75bd3ef63f2c0f6fd0a87e3f8d4809a38ac955b7d97fd4af1bd2c882999d5c"'


(Note-to-self) Would love to better understand how snapshots are generated and pasted here (practically). I read about Snapshot testing here: https://jestjs.io/docs/snapshot-testing#inline-snapshots.

It sounds like these are generated "automagically":

First, you write a test, calling .toMatchInlineSnapshot() with no arguments

The next time you run Jest, tree will be evaluated, and a snapshot will be written as an argument to toMatchInlineSnapshot

By default, Jest handles the writing of snapshots into your source code. However, if you're using prettier in your project, Jest will detect this and delegate the work to prettier instead (including honoring your configuration).

while it doesn't differ much on the snapshot front, viem is using vitest for testing rather than jest.

arthurgousset · 2023-11-01T15:29:38Z

src/chains/celo/formatters.test.ts

Nit: (not related to changes in this PR)

Based on this recent Slack convo with @mcortesi, transaction receipts do not include feeCurrency, gatewayFee, or gatewayFeeRecipient fields, which means they are undefined in every transaction receipt object (by default). We can thus look into removing them from the expected and actual transaction receipt (in tests), because they will always trivially be equal.

I suggest we fix this in a separate PR to keep this one clean and get the regression fix merged as soon as possible.

describe('transactionReceipt', () => { test('formatter', () => { const { transactionReceipt } = celo.formatters! expect( transactionReceipt.format({ blockHash: '0x89644bbd5c8d682a2e9611170e6c1f02573d866d286f006cbf517eec7254ec2d', blockNumber: '0x1', contractAddress: '0xa152f8bb749c55e9943a3a0a3111d18ee2b3f94e', cumulativeGasUsed: '0x2', effectiveGasPrice: '0x3', - feeCurrency: null, from: '0xa152f8bb749c55e9943a3a0a3111d18ee2b3f94e', gasUsed: '0x4', - gatewayFee: null, - gatewayFeeRecipient: null, logs: [], to: '0x15d4c048f83bd7e37d49ea4c83a07267ec4203da', status: '0x0', type: '0x0', }), ).toMatchInlineSnapshot(` { "blockHash": "0x89644bbd5c8d682a2e9611170e6c1f02573d866d286f006cbf517eec7254ec2d", "blockNumber": 1n, "contractAddress": "0xa152f8bb749c55e9943a3a0a3111d18ee2b3f94e", "cumulativeGasUsed": 2n, "effectiveGasPrice": 3n, - "feeCurrency": null, "from": "0xa152f8bb749c55e9943a3a0a3111d18ee2b3f94e", "gasUsed": 4n, - "gatewayFee": null, - "gatewayFeeRecipient": null, "logs": [], "status": "reverted", "to": "0x15d4c048f83bd7e37d49ea4c83a07267ec4203da", "transactionIndex": null, "type": "legacy", } `)

arthurgousset · 2023-11-01T15:40:15Z

src/chains/celo/formatters.ts

Nit: (not related to changes in this PR)

Based on this recent Slack convo with @mcortesi, transaction receipts do not include feeCurrency, gatewayFee, or gatewayFeeRecipient fields, which means they are undefined in every transaction receipt object (by default). We can thus look into removing them from here

transactionReceipt: /*#__PURE__*/ defineTransactionReceipt({ format( args: CeloRpcTransactionReceiptOverrides, ): CeloTransactionReceiptOverrides { return { - feeCurrency: args.feeCurrency, - gatewayFee: args.gatewayFee ? hexToBigInt(args.gatewayFee) : null, - gatewayFeeRecipient: args.gatewayFeeRecipient, } }, }),

I suggest we fix this in a separate PR to keep this one clean and get the regression fix merged as soon as possible.

arthurgousset · 2023-11-01T15:47:32Z

src/chains/celo/utils.ts

+
+export function isEmpty(
+  value: string | undefined | number | BigInt,
+): value is undefined {


(note-to-self) Neat! Learnt about "user-defined type guards" e.g. value is undefined.

arthurgousset · 2023-11-01T15:50:23Z

src/chains/celo/utils.ts

+  return (
+    isEIP1559(transaction) &&
+    isPresent(transaction.feeCurrency) &&
+    isEmpty(transaction.gatewayFee) &&
+    isEmpty(transaction.gatewayFeeRecipient)
+  )


This is neat, makes it a lot more readable. Thanks for refactoring this!

arthurgousset · 2023-11-02T09:33:43Z

src/chains/celo/serializers.ts

  ) {
    throw new BaseError(
      '`gatewayFee` and `gatewayFeeRecipient` must be provided together.',
    )
  }

-  if (feeCurrency && !feeCurrency?.startsWith('0x')) {
+  if (isPresent(feeCurrency) && !isAddress(feeCurrency)) {


This reads so much better with isPresent and isAddress, thanks for refactoring this!

arthurgousset

Not the strongest approval since, I haven't worked in viem before, but this looks good to me. Love the refactored use of utils.

nicolasbrugneaux · 2023-11-02T10:23:04Z

Note for reviewers, we paired with @0xarthurxyz to run the build locally and try if the failing transactions were fixed, and they were.

src/chains/celo/serializers.ts

aaronmgdr · 2023-11-02T10:40:11Z

I see viem defines the EIP1559 TX like

export type TransactionSerializableEIP1559<
  TQuantity = bigint,
  TIndex = number,
> = TransactionSerializableBase<TQuantity, TIndex> &
  Partial<FeeValuesEIP1559<TQuantity>> & {
    accessList?: AccessList
    chainId: number
    type?: 'eip1559'
    yParity?: number
  }

note the Partial<FeeValuesEIP1559<TQuantity>> but in the Celo Serializable TX types it is FeeValuesEIP1559<TQuantity>

My gut is they should be the same

export type TransactionSerializableCIP42<
  TQuantity = bigint,
  TIndex = number,
> = TransactionSerializableBase<TQuantity, TIndex> &
  FeeValuesEIP1559<TQuantity> & {
    accessList?: AccessList
    feeCurrency?: Address
    gatewayFeeRecipient?: Address
    gatewayFee?: TQuantity
    chainId: number
    type?: 'cip42'
  }

export type TransactionSerializableCIP64<
  TQuantity = bigint,
  TIndex = number,
> = TransactionSerializableBase<TQuantity, TIndex> &
  FeeValuesEIP1559<TQuantity> & {
    accessList?: AccessList
    feeCurrency?: Address
    chainId: number
    type?: 'cip64'
  }

* Fix: cip64 regressions (#6) * fix: refactor cip64 to be more robust * fix: types * fix: signTransaction tests * refactor: PR feedback * chore: add changeset * test: add celo/utils test coverage (#8) * CELO utils test coverage * test: generate new address for each test --------- Co-authored-by: Nicolas Brugneaux <nicolas.brugneaux@gmail.com> * chore: update src/chains/celo/utils.ts Co-authored-by: jxom <jakemoxey@gmail.com> * Update yellow-eggs-compare.md --------- Co-authored-by: Leszek Stachowski <leszkostachowski@gmail.com> Co-authored-by: jxom <jakemoxey@gmail.com>

nicolasbrugneaux force-pushed the fix/celo-txs branch from fb17e68 to 638714f Compare October 30, 2023 18:03

aaronmgdr reviewed Oct 31, 2023

View reviewed changes

src/chains/celo/formatters.test.ts Show resolved Hide resolved

aaronmgdr reviewed Oct 31, 2023

View reviewed changes

src/chains/celo/serializers.test.ts Show resolved Hide resolved

aaronmgdr reviewed Oct 31, 2023

View reviewed changes

src/chains/celo/serializers.test.ts Outdated Show resolved Hide resolved

aaronmgdr reviewed Oct 31, 2023

View reviewed changes

src/chains/celo/utils.ts Outdated Show resolved Hide resolved

nicolasbrugneaux mentioned this pull request Oct 31, 2023

Fix bug that initiates an EIP-1559 tx instead of a CIP-64 tx since viem@1.180 celo-org/txtypes#1

Closed

nicolasbrugneaux force-pushed the fix/celo-txs branch 2 times, most recently from 3835187 to 3e41cd9 Compare October 31, 2023 17:07

fix: refactor cip64 to be more robust

ed70912

nicolasbrugneaux changed the title ~~wip: tx types fixin~~ Fix: cip64 regressions Oct 31, 2023

nicolasbrugneaux force-pushed the fix/celo-txs branch from 3e41cd9 to ed70912 Compare October 31, 2023 17:12

nicolasbrugneaux self-assigned this Oct 31, 2023

aaronmgdr reviewed Oct 31, 2023

View reviewed changes

src/chains/celo/utils.ts Outdated Show resolved Hide resolved

fix: types

27c1794

nicolasbrugneaux force-pushed the fix/celo-txs branch 2 times, most recently from bd6e922 to 86e52cb Compare October 31, 2023 17:57

arthurgousset reviewed Nov 1, 2023

View reviewed changes

fix: signTransaction tests

bc31c3f

nicolasbrugneaux force-pushed the fix/celo-txs branch from 86e52cb to bc31c3f Compare November 2, 2023 08:06

arthurgousset reviewed Nov 2, 2023

View reviewed changes

arthurgousset self-requested a review November 2, 2023 09:45

arthurgousset approved these changes Nov 2, 2023

View reviewed changes

aaronmgdr reviewed Nov 2, 2023

View reviewed changes

src/chains/celo/serializers.ts Show resolved Hide resolved

refactor: PR feedback

21a156f

nicolasbrugneaux merged commit 4d0c7a2 into main Nov 2, 2023
6 of 22 checks passed

nicolasbrugneaux mentioned this pull request Nov 2, 2023

Fix: celo-specific transaction (CIP64) regressions wevm/viem#1434

Merged

nicolasbrugneaux deleted the fix/celo-txs branch November 2, 2023 13:17

shazarre restored the fix/celo-txs branch November 9, 2023 13:12

shazarre deleted the fix/celo-txs branch November 15, 2023 14:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: cip64 regressions #6

Fix: cip64 regressions #6

nicolasbrugneaux commented Oct 30, 2023 •

edited

github-actions bot commented Oct 30, 2023 •

edited

arthurgousset Nov 1, 2023

nicolasbrugneaux Nov 2, 2023

arthurgousset Nov 1, 2023 •

edited

arthurgousset Nov 1, 2023

arthurgousset Nov 1, 2023 •

edited

arthurgousset Nov 1, 2023

arthurgousset Nov 2, 2023

arthurgousset left a comment

nicolasbrugneaux commented Nov 2, 2023 •

edited

aaronmgdr commented Nov 2, 2023

		const snapshot =
		'"0x7cf8840182031184773594008504a817c80082520894765de816845861e75a25fca122bb6898b8b1282a940f16e9b0d03470827a95cdfd0cb8a8a3b46969b904808080c001a062bee7f81cccd1f430b4b66ec5a23737d6fbee9965e63ac582d09f63aef32bdca05e75bd3ef63f2c0f6fd0a87e3f8d4809a38ac955b7d97fd4af1bd2c882999d5c"'

Fix: cip64 regressions #6

Fix: cip64 regressions #6

Conversation

nicolasbrugneaux commented Oct 30, 2023 • edited

github-actions bot commented Oct 30, 2023 • edited

size-limit report 📦

arthurgousset Nov 1, 2023

Choose a reason for hiding this comment

nicolasbrugneaux Nov 2, 2023

Choose a reason for hiding this comment

arthurgousset Nov 1, 2023 • edited

Choose a reason for hiding this comment

arthurgousset Nov 1, 2023

Choose a reason for hiding this comment

arthurgousset Nov 1, 2023 • edited

Choose a reason for hiding this comment

arthurgousset Nov 1, 2023

Choose a reason for hiding this comment

arthurgousset Nov 2, 2023

Choose a reason for hiding this comment

arthurgousset left a comment

Choose a reason for hiding this comment

nicolasbrugneaux commented Nov 2, 2023 • edited

aaronmgdr commented Nov 2, 2023

nicolasbrugneaux commented Oct 30, 2023 •

edited

github-actions bot commented Oct 30, 2023 •

edited

arthurgousset Nov 1, 2023 •

edited

arthurgousset Nov 1, 2023 •

edited

nicolasbrugneaux commented Nov 2, 2023 •

edited