search

celo-org / viem

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

fix: CIP-64 are sent as EIP-1559 transactions #9

Closed shazarre closed 1 year ago

shazarre commented 1 year ago

This PR fixes bug where CIP-64 transactions are sent as EIP-1559 transactions.

It reverts code changes to src/chains/celo/utils.ts from this commit and repurposes a test that started failing because of the change.

Context

When prepareTransactionRequest is called and type is not explicitly provided then it will be determined by getTransactionType and because for CIP-64 all EIP-1559 fields are provided it will be set to eip1559 as per:

if (
    typeof transaction.maxFeePerGas !== 'undefined' ||
    typeof transaction.maxPriorityFeePerGas !== 'undefined'
  )
    return 'eip1559' as GetTransactionType<TTransactionSerializable>

Previously the logic of "forcing" the type in src/chains/celo/utils.ts allowed it only if the type was explicitly set to cip64 and otherwise it would try to determine if it's a CIP-64 transaction based on the provided fields:

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

After the aforementioned commit it won't ever get to determine the type based on the fields as it will always go into if (transaction.type) branch as the eip1559 type is injected hence resulting in sending wrong transaction type to the blockchain.

github-actions[bot] commented 1 year ago

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
viem (esm) 58.88 KB (0%) 1.2 s (0%) 684 ms (+64.53% 🔺) 1.9 s
viem (cjs) 77.78 KB (0%) 1.6 s (0%) 824 ms (-1.44% 🔽) 2.4 s
viem (minimal surface - tree-shaking) 3.89 KB (0%) 78 ms (0%) 164 ms (+47.38% 🔺) 242 ms
viem/accounts 89.06 KB (0%) 1.8 s (0%) 269 ms (+39.25% 🔺) 2.1 s
viem/accounts (tree-shaking) 19.41 KB (0%) 389 ms (0%) 334 ms (+83.65% 🔺) 722 ms
viem/actions 44.25 KB (0%) 886 ms (0%) 342 ms (+19.02% 🔺) 1.3 s
viem/actions (tree-shaking) 350 B (0%) 10 ms (0%) 107 ms (-34.74% 🔽) 117 ms
viem/chains 19.25 KB (-0.06% 🔽) 385 ms (-0.06% 🔽) 307 ms (+7.08% 🔺) 692 ms
viem/chains (tree-shaking) 470 B (0%) 10 ms (0%) 95 ms (-17.85% 🔽) 105 ms
viem/chains/utils 9.26 KB (-0.09% 🔽) 186 ms (-0.09% 🔽) 250 ms (+98.64% 🔺) 435 ms
viem/chains/utils (tree-shaking) 5.36 KB (-0.15% 🔽) 108 ms (-0.15% 🔽) 78 ms (-36.48% 🔽) 185 ms
viem/ens 44.25 KB (-0.01% 🔽) 886 ms (-0.01% 🔽) 235 ms (-32.47% 🔽) 1.2 s
viem/ens (tree-shaking) 18.3 KB (0%) 367 ms (0%) 104 ms (-45.34% 🔽) 470 ms
aaronmgdr commented 1 year ago

Its probably more complex but i think what we really need is more of an integration style test that goes all the way from sending to to end

(the code below isnt correct but i think you get the idea


const transaction = await client.sendTransaction({feeCurrency: '0x01...'})

expect(parseTransaction(transaction).type).toequal('0x7b')