celo-org / viem

TypeScript Interface for Ethereum
https://viem.sh
Other
0 stars 0 forks source link

Fix: cip64 regressions #6

Closed nicolasbrugneaux closed 1 year ago

nicolasbrugneaux commented 1 year ago

This PR aims to fix the issues reported by @0xarthurxyz in https://github.com/celo-org/txtypes/issues/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[bot] commented 1 year ago

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
nicolasbrugneaux commented 1 year ago

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

aaronmgdr commented 1 year ago

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