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

Deprecates Celo legacy transaction type (0) #20

Merged

Conversation

arthurgousset
Copy link
Collaborator

@arthurgousset arthurgousset commented Apr 16, 2024

Description

Goal is to deprecate the Celo legacy transaction type (0).

Context: [Action Required] Celo Legacy TX type deprecation

Changes

  • Deprecates Celo legacy transaction type (0), instead
    • when a TX pays for gas in CELO, then an Ethereum-compatible transaction is created
    • when a TX pays for gas in ERC-20 tokens, then a CIP64 transaction is created
  • Deletes any trace of gatewayFee and gatewayFeeRecipient

Other changes

  • Bumps required node version from ">=10" to ">=18.14.2" 8da85b4
  • Adds CONTRIBUTING.md file with instructions for first-time contributors to run tests
  • Adds .env file and example to run test suite for first-time contributors
  • Updates Celoscan API URL for Alfajores and fixes tests (in history.test.ts) related to CeloscanProvider that were first added here Add CeloscanProvider #8
  • Combines deploy.test.ts and transfers.test.ts in a single file transactions.test.tsto prevent flakey tests due to simultaneous nonce fetching and setting.

Related

This is the current long-term support (LTS) version. Source: https://endoflife.date/nodejs
This file is copied by first-time users and they paste their secret mnemonic phrase here.
Because the environment variable is located at `tests/.env.test.local` dotenv has to be configured to look for the correct file.

By default it looks for `.env` in the root of the directory.
Before this, tests had to be run manually with an environment variable `NETWORK`, and the test was run against the user submitted address.

Removes the need for `NETWORK` by adding 3 distinct test cases for each network.

Removes dependency on user submitted address, by hardcoding 3 address that definitely have a transaction history on the respective network.

This ensures the test asserts the desired behaviour and doesn't depend on user submitted values.
For ease of reference for future readers.
This test case is a duplicate of a test available in `transfer.test.ts`.
@arthurgousset
Copy link
Collaborator Author

You can check type and ethCompatible fields in the RPC response of the eth_getTransactionByHash method for your transaction. If type is 0x0 and ethCompatible is false, it means that you’re using Celo Legacy Type 0 transactions and need to migrate.

Source: [Action Required] Celo Legacy TX type deprecation

@arthurgousset
Copy link
Collaborator Author

arthurgousset commented Apr 17, 2024

Note-to-self:

Ethers doesn't seem to parse the ethCompatible field in RPC responses when calling the eth_getTransactionByHash method on Celo full nodes.

  1. The provider.getTransaction method

    const rpcResponse = await provider.getTransaction('0x669f7704413672f348b1a70b095ec64bea3d201214849007a180e22ae9b2498b')
    console.log(rpcResponse)

    does not include the ethCompatible field:

    TransactionResponse {
      provider: JsonRpcProvider {},
      blockNumber: 24914183,
      blockHash: '0x49f734d16553c13d079fb92c603b5bc28b4686ab39f130b30a4a147e927ea87a',
      index: undefined,
      hash: '0x669f7704413672f348b1a70b095ec64bea3d201214849007a180e22ae9b2498b',
      type: 0,
      to: '0xefB84935239dAcdecF7c5bA76d8dE40b077B7b33',
      from: '0xB947c54BE882314623ee3D74684d0d785dD50335',
      nonce: 49738,
      gasLimit: 553612n,
      gasPrice: 7500000000n,
      maxPriorityFeePerGas: null,
      maxFeePerGas: null,
      data: '0x80e5074400000000000000000000000073f93dcc49cb8a239e2032663e9475dd5ef29a08000000000000000000000000000000000000000002240d0c26ad408b890f9ac0000000000000000000000000b7614f7174a07028a5ff5e1adc68a031b646857f000000000000000000000000a97dbefac6026f93cc5714c4c150b7466e9502ef',
      value: 0n,
      chainId: 42220n,
      signature: Signature { r: "0x8622bcde4a590097e74f2222ac1f068a84893241c410bda2b553075f2544aaf8", s: "0x49e28bdfe420ea58d689a9a0a2b25820f99a9df6c7b3bad2b08953036f3d9303", yParity: 1, networkV: 84476 },
      accessList: null
    }
  2. While a network request to the node:

    const url = 'https://forno.celo.org';
        const headers = {
            'Content-Type': 'application/json'
        };
        const body = JSON.stringify({
            jsonrpc: '2.0',
            method: 'eth_getTransactionByHash',
            params: ['0x669f7704413672f348b1a70b095ec64bea3d201214849007a180e22ae9b2498b'],
            id: 1
        });
    
        try {
            const response = await fetch(url, {
                method: 'POST',
                headers: headers,
                body: body
            });
            const data = await response.json();
            console.log(data);
        } catch (error) {
            console.error('Failed to fetch data:', error);
        }

    does include the ethCompatible field:

    {
      jsonrpc: '2.0',
      id: 1,
      result: {
        blockHash: '0x49f734d16553c13d079fb92c603b5bc28b4686ab39f130b30a4a147e927ea87a',
        blockNumber: '0x17c2907',
        from: '0xb947c54be882314623ee3d74684d0d785dd50335',
        gas: '0x8728c',
        gasPrice: '0x1bf08eb00',
        feeCurrency: null,
        gatewayFeeRecipient: null,
        gatewayFee: '0x0',
        hash: '0x669f7704413672f348b1a70b095ec64bea3d201214849007a180e22ae9b2498b',
        input: '0x80e5074400000000000000000000000073f93dcc49cb8a239e2032663e9475dd5ef29a08000000000000000000000000000000000000000002240d0c26ad408b890f9ac0000000000000000000000000b7614f7174a07028a5ff5e1adc68a031b646857f000000000000000000000000a97dbefac6026f93cc5714c4c150b7466e9502ef',
        nonce: '0xc24a',
        to: '0xefb84935239dacdecf7c5ba76d8de40b077b7b33',
        transactionIndex: '0x4',
        value: '0x0',
        type: '0x0',
        v: '0x149fc',
        r: '0x8622bcde4a590097e74f2222ac1f068a84893241c410bda2b553075f2544aaf8',
        s: '0x49e28bdfe420ea58d689a9a0a2b25820f99a9df6c7b3bad2b08953036f3d9303',
        ethCompatible: false
      }
    }

Specifically any mention of `gatewayFee` or `gatewayFeeRecipient`.

Deletes Celo Legacy Transaction interface
The first if statement seems redundant, because if `isCIP64(tx)` is true, then
1. there cannot be a tx.gatewayFee,
2. there cannot be a tx.gatewayFeeRecipient
3. there cannot be a tx.gasPrice

It might have made sense to keep the code that deletes the fields in case `isCIP64(tx)` doesn't work as expected. But that adds more complexity than necessary in my opinion.

Un-nesting if, else if, else statement (personal preference).
From cUSD to USDC transfer.
The test was and is still incomplete, because it was and still is only testing that an ERC-20 transfer can be made.

It doesn't assert that a USDC transfer with USDC as fee currency can be made.

Moved this test into the CIP-64 block for ease of reference.
Commenting this out for the time being, because the tests are not relevant with Celo Legacy transactions deprecated.
Adds tests to assert code changes in the library will generate actively supported transaction types.

The tests are not perfect, and need to be polished.
@aaronmgdr
Copy link
Collaborator

aaronmgdr commented Apr 18, 2024

/**
* TODO(Arthur): gatewayFee and gatewayFeeRecipient are not supported anymore
*/
// This order should match the order in Geth.
// https://github.com/celo-org/celo-blockchain/blob/027dba2e4584936cc5a8e8993e4e27d28d5247b8/core/types/transaction.go#L65
raw = [
toBeHex(tx.nonce!),
tx.gasPrice ? toBeHex(tx.gasPrice) : "0x",
tx.gasLimit ? toBeHex(tx.gasLimit) : "0x",
tx.feeCurrency || "0x",
tx.gatewayFeeRecipient || "0x",
tx.gatewayFee ? toBeHex(tx.gatewayFee) : "0x",
tx.to || "0x",
tx.value ? toBeHex(tx.value) : "0x",
tx.data || "0x",
];

this default case is for celo type 0. it must go.

what options do we have as an alternative default.

  • implement ethereum type 0
  • throw an error telling people they must submit either a eip1559 or cip64 transaction or create transaction using ethers normal

@arthurgousset
Copy link
Collaborator Author

Note-to-self:

When a transaction only specifies to, value, and feeCurrency do we want that transaction to

  1. drop feeCurrency and become a type 0 transaction with gasPrice automatically estimated,
  2. automagically add maxFeePerGas, maxPriorityFeePerGas, and gasLimit , or
  3. TypeScript complains that feeCurrency without specifying maxFeePerGas, maxPriorityFeePerGas, and gasLimit is not allowed.

For example:

const txResponse = await signer.sendTransaction({
    to: signer.address,
    value: 1n,
    feeCurrency: USDC_ADAPTER_ALFAJORES_ADDRESS,
});

At the moment, it's (1.), it drops feeCurrency and becomes a type 0 Ethereum legacy transaction.
Before my changes, it added gasPrice and turned it into a type 0 Celo legacy transaction. But that is now deprecated.

@arthurgousset
Copy link
Collaborator Author

arthurgousset commented Apr 18, 2024

/**
* TODO(Arthur): gatewayFee and gatewayFeeRecipient are not supported anymore
*/
// This order should match the order in Geth.
// https://github.com/celo-org/celo-blockchain/blob/027dba2e4584936cc5a8e8993e4e27d28d5247b8/core/types/transaction.go#L65
raw = [
toBeHex(tx.nonce!),
tx.gasPrice ? toBeHex(tx.gasPrice) : "0x",
tx.gasLimit ? toBeHex(tx.gasLimit) : "0x",
tx.feeCurrency || "0x",
tx.gatewayFeeRecipient || "0x",
tx.gatewayFee ? toBeHex(tx.gatewayFee) : "0x",
tx.to || "0x",
tx.value ? toBeHex(tx.value) : "0x",
tx.data || "0x",
];

this default case is for celo type 0. it must go.

what options do we have as an alternative default.

  • implement ethereum type 0
  • throw an error telling people they must submit either a eip1559 or cip64 transaction or create transaction using ethers normal

That's done in c888262, I hadn't pushed my changes yet.

Replaces it with Ethereum legacy tx type (0) at the moment.
I left a note about my current thinking in the code. I'm not sure this is the best/appropriate fall-back option.
Current 2 tests are failing for reasons that are acceptable (type is `0` and not `2`), but Ethereum-compatible. So making sure the transactions are type `2` instead of `0` is a "nice to have".

There is a nonce related bug that only occurs when I run the entire test suite. Otherwise, the test doesn't fail. It's a flakey test.
- moves tests from `deploy.test.ts` to `transfer.test.ts`
- renames file from `transfer.test.ts` to `transactions.test.ts`

I moved the tests into a single file, because running them concurrently made the tests flakey. Specifically either the contract deployment or the first transfer test would fail because of a nonce issue.

I expect it's because both tests query `eth_getTransactionCount` and get the same result milliseconds apart. So they submit transactions with the same nonce.
@arthurgousset
Copy link
Collaborator Author

arthurgousset commented Apr 22, 2024

At this point, the main code changes are made:

  1. when a TX pays for gas in CELO, then an Ethereum-compatible transaction is created
  2. when a TX pays for gas in ERC-20 tokens, then a CIP64 transaction is created

There are a couple outstanding (potential) issues:

  1. [Edit: This is done ✅] [nice to have] when a TX pays for gas in CELO, then a type 0 transaction is constructed, when instead a type 2 transaction would be preferable.

    This happens when no gas fields are specified. For example, these two test cases are generating type 0 transactions instead of type 2 and are failing for that reason:

    const txResponse = await signer.sendTransaction({
    to: signer.address,
    value: BigInt(1),
    });

    const txResponse = await usdc.transfer(signer.address, 1n);

    👉 This doesn't relate to the scope of this PR, since it doesn't interfere with deprecating the Celo legacy transaction type.

  2. [Edit: This is done ✅] [nice to have] when I call eth_gasTransactionByHash on an EIP-1559 transaction (i.e. type 2), the response object shows "ethCompatible": false. Intuitively I would expect that to be "ethCompatible": true since type 2 (EIP1559) is an Ethereum-compatible type.

    For example:

    $ curl -s -X POST -H "Content-Type: application/json" --data '{"jsonrpc":"2.0","method":"eth_getTransactionByHash","params":["0x04d8b58785bdd7199469489607e9f1e61be88ff115cb9c8f6b48b286effcda59"],"id":1}' "https://alfajores-forno.celo-testnet.org/" | jq
    
    {
      "jsonrpc": "2.0",
      "id": 1,
      "result": {
        "blockHash": "0x260a8b8d0ab8fa4b71d9f2ef303b020d009f3f8ba167923f3cc3b3122a6ad6c4",
        "blockNumber": "0x169612a",
        "from": "0xae4ca7989e35e505948ea5faeb054f055567e50c",
        "gas": "0x8aa2",
        "gasPrice": "0x37e11d600",
        "maxFeePerGas": "0x6fc23ac00",
        "maxPriorityFeePerGas": "0x2540be400",
        "feeCurrency": null,
        "gatewayFeeRecipient": null,
        "gatewayFee": "0x0",
        "hash": "0x04d8b58785bdd7199469489607e9f1e61be88ff115cb9c8f6b48b286effcda59",
        "input": "0x",
        "nonce": "0xdb",
        "to": "0xae4ca7989e35e505948ea5faeb054f055567e50c",
        "transactionIndex": "0x0",
        "value": "0x1",
        "type": "0x2",
        "accessList": [],
        "chainId": "0xaef3",
        "v": "0x0",
        "r": "0xa3925714aabcf85921eb9a380de3e1243d112f0ccd0155941eb83c26ccc16eed",
        "s": "0x5111d0ffb7464b6ae1bcc192b86928b0f21ec1bc8cf6ed5bc7f2b4a017127e42",
        "ethCompatible": false
      }
    }

    I asked the Blockchain team on Slack to clarify that this is expected.

    👉 This doesn't relate to the scope of this PR, since it doesn't interfere with deprecating the Celo legacy transaction type.

  3. [Edit: This is done ✅] [nice to have] refactorisCIP64 and isFeeCurrency into a single function.

    At the moment, I defined two functions to avoid introducing too many areas for new bugs. But, if the current implementation satisfies the tests and we are comfortable with the current test coverage, we can refactor the functions to make the code base simpler.

    export function isCIP64(tx: any) {
    return (
    isEIP1559(tx) &&
    isPresent(tx.feeCurrency) &&
    !isPresent(tx.gatewayFeeRecipient) &&
    !isPresent(tx.gatewayFee)
    );
    }

    export function isFeeCurrency(tx: any): boolean {
    return (
    isPresent(tx.feeCurrency) &&
    !isPresent(tx.gatewayFeeRecipient) &&
    !isPresent(tx.gatewayFee)
    )
    }

    👉 This doesn't relate to the scope of this PR, since it doesn't interfere with deprecating the Celo legacy transaction type.

@arthurgousset arthurgousset marked this pull request as ready for review April 22, 2024 22:18
src/lib/CeloProvider.ts Outdated Show resolved Hide resolved
tests/consts.ts Outdated Show resolved Hide resolved
ethCompatible flag is only every true for legacy txs (where we need it to tell Celo and Ethereum legacy txs apart). Type 2 txs are Ethereum compatible by definition. So testing the `type` is sufficient to infer Ethereum-compatibility.
@arthurgousset
Copy link
Collaborator Author

arthurgousset commented Apr 23, 2024

  1. [nice to have] when I call eth_gasTransactionByHash on an EIP-1559 transaction (i.e. type 2), the response object shows "ethCompatible": false. Intuitively I would expect that to be "ethCompatible": true since type 2 (EIP1559) is an Ethereum-compatible type.
    For example:

    $ curl -s -X POST -H "Content-Type: application/json" --data '{"jsonrpc":"2.0","method":"eth_getTransactionByHash","params":["0x04d8b58785bdd7199469489607e9f1e61be88ff115cb9c8f6b48b286effcda59"],"id":1}' "https://alfajores-forno.celo-testnet.org/" | jq
    
    {
      "jsonrpc": "2.0",
      "id": 1,
      "result": {
        "blockHash": "0x260a8b8d0ab8fa4b71d9f2ef303b020d009f3f8ba167923f3cc3b3122a6ad6c4",
        "blockNumber": "0x169612a",
        "from": "0xae4ca7989e35e505948ea5faeb054f055567e50c",
        "gas": "0x8aa2",
        "gasPrice": "0x37e11d600",
        "maxFeePerGas": "0x6fc23ac00",
        "maxPriorityFeePerGas": "0x2540be400",
        "feeCurrency": null,
        "gatewayFeeRecipient": null,
        "gatewayFee": "0x0",
        "hash": "0x04d8b58785bdd7199469489607e9f1e61be88ff115cb9c8f6b48b286effcda59",
        "input": "0x",
        "nonce": "0xdb",
        "to": "0xae4ca7989e35e505948ea5faeb054f055567e50c",
        "transactionIndex": "0x0",
        "value": "0x1",
        "type": "0x2",
        "accessList": [],
        "chainId": "0xaef3",
        "v": "0x0",
        "r": "0xa3925714aabcf85921eb9a380de3e1243d112f0ccd0155941eb83c26ccc16eed",
        "s": "0x5111d0ffb7464b6ae1bcc192b86928b0f21ec1bc8cf6ed5bc7f2b4a017127e42",
        "ethCompatible": false
      }
    }

    I asked the Blockchain team on Slack to clarify that this is expected.
    👉 This doesn't relate to the scope of this PR, since it doesn't interfere with deprecating the Celo legacy transaction type.

Based on the response from the Blockchain team, it's expected that ethCompatible is false for EIP-1559 transactions ✅ Type 2 txs are Ethereum compatible by definition. The ethCompatible flag is only every true for legacy txs (where we need it to tell Celo and Ethereum legacy txs apart). Effectively, it's a legacy reason. In fact, the Blockchain team opened a PR to change this:

For that reason, I removed the ethCompatible test on EIP1559 transactions in 1f4d587

@arthurgousset
Copy link
Collaborator Author

arthurgousset commented Apr 23, 2024

  1. [nice to have] when a TX pays for gas in CELO, then a type 0 transaction is constructed, when instead a type 2 transaction would be preferable.
    This happens when no gas fields are specified. For example, these two test cases are generating type 0 transactions instead of type 2 and are failing for that reason:
    const txResponse = await signer.sendTransaction({
    to: signer.address,
    value: BigInt(1),
    });

const txResponse = await usdc.transfer(signer.address, 1n);

👉 This doesn't relate to the scope of this PR, since it doesn't interfere with deprecating the Celo legacy transaction type.

This is done ✅ As of ed7dfbe and c48bd5e, Ethereum-compatible transactions are EIP-1559 (type 2) by default, unless gasPrice is explicitly set, in which case they are type 0. Thanks for your help @nicolasbrugneaux !

@jmrossy
Copy link
Owner

jmrossy commented Apr 23, 2024

Thanks @arthurgousset @aaronmgdr for your great work on this!
Once you've resolved aaron's feedback and he's approved, I'll look over it as well. Busy the next few days but should have time this wkend.

@arthurgousset
Copy link
Collaborator Author

arthurgousset commented Apr 23, 2024

Thanks @arthurgousset @aaronmgdr for your great work on this! Once you've resolved aaron's feedback and he's approved, I'll look over it as well. Busy the next few days but should have time this wkend.

Sounds good, I'll let you know when it's ready @jmrossy 👍

@aaronmgdr
Copy link
Collaborator

except for the conversation about gas price. all other files look good to me

@arthurgousset
Copy link
Collaborator Author

except for the conversation about gas price. all other files look good to me

Let's wait for some more guidance and context from the Blockchain team in that case.

@arthurgousset
Copy link
Collaborator Author

@jmrossy I resolved the fee estimation conversation and committed subsequent changes.
This is ready for review ✅

Copy link
Owner

@jmrossy jmrossy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! These changes look good to me.
The CI is failing but I think it's because PRs from forks can't access the repo secrets (the mnemonic).

If you confirm you're done with your changes, I'll merge this into a feature branch and confirm the CI is still happy.

@arthurgousset
Copy link
Collaborator Author

Nice work! These changes look good to me. The CI is failing but I think it's because PRs from forks can't access the repo secrets (the mnemonic).

If you confirm you're done with your changes, I'll merge this into a feature branch and confirm the CI is still happy.

I'm done with the changes 👍 Let's see what CI says :)

@jmrossy jmrossy changed the base branch from master to 2.1.0 May 2, 2024 14:16
@jmrossy jmrossy merged commit ac0d6f5 into jmrossy:2.1.0 May 2, 2024
1 check failed
@arthurgousset arthurgousset deleted the arthurgousset/deprecate-celo-legacy-transaction branch May 2, 2024 14:48
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.

Deprecate celo legacy transaction type (0) and CIP42 type (124)
4 participants