Deprecate Celo legacy transaction type (0)2.1.0 #21
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
Celoscan's current Alfajores endpoint URL can be found here: https://docs.celoscan.io/getting-started/endpoint-urls
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`.
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.
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.
Most are replicated elsewhere, some are not relevant anymore
- deletes legacy fields - linting
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.
asserts it's EIP1559, but fails at `ethCompatible`
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.
…Price` Also renames gas variables in fee currency case for better readability.
…acy-transaction Deprecates Celo legacy transaction type (0)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See #20