Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
Nov 15, 2023

Conversation

shazarre
Copy link

@shazarre shazarre commented Nov 9, 2023

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.

Copy link

github-actions bot commented Nov 9, 2023

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
Copy link
Member

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


jeanregisser added a commit to valora-inc/wallet that referenced this pull request Nov 10, 2023
…1559 (#4458)

### Description

This reverts commit 3df1888
(#4448).

This is to fix CIP-64 TXs serialized as EIP-1559 when using the
`prepareTransactionRequest` method from viem.
Essentially preventing to pay for TXs using a `feeCurrency`.
This was affecting only the "send with viem" flow as it's the only one
using that method.

We can upgrade again when celo-org/viem#9 lands.

I updated our Renovate rules to ignore `viem` for now.

### Test plan

- Manually tested an account with only cUSD can successfully send TXs.
- Would be good to cover this with an e2e test, but for now I'm only
addressing the immediate issue for the release.

### Related issues

- See Slack
[thread](https://valora-app.slack.com/archives/C04B61SJ6DS/p1699612457370369)

### Backwards compatibility

Yes
jeanregisser added a commit to valora-inc/wallet that referenced this pull request Nov 10, 2023
…1559 (#4458)

### Description

This reverts commit 3df1888
(#4448).

This is to fix CIP-64 TXs serialized as EIP-1559 when using the
`prepareTransactionRequest` method from viem.
Essentially preventing to pay for TXs using a `feeCurrency`.
This was affecting only the "send with viem" flow as it's the only one
using that method.

We can upgrade again when celo-org/viem#9 lands.

I updated our Renovate rules to ignore `viem` for now.

### Test plan

- Manually tested an account with only cUSD can successfully send TXs.
- Would be good to cover this with an e2e test, but for now I'm only
addressing the immediate issue for the release.

### Related issues

- See Slack
[thread](https://valora-app.slack.com/archives/C04B61SJ6DS/p1699612457370369)

### Backwards compatibility

Yes
@shazarre shazarre merged commit 30a964e into main Nov 15, 2023
12 of 23 checks passed
arthurgousset pushed a commit that referenced this pull request Dec 19, 2023
* fix: CIP-64 are sent as EIP-1559 transactions (#9)

* fix: CIP-64 are sent as EIP-1559 transactions
* fix: EIP-1159 -> EIP-1559 typo
* chore: add test case for CIP-64 recognition when eip1559 type is provided
* chore: introduce a sendTransaction Celo test to verify CIP-64 being sent instead of EIP-1559
* chore: refactor and simplify sendTransaction test
* chore: DRY
* chore: add a comment for CIP-64 check

* chore: add changeset
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants