Update to Cadence v1.2.1 #623
Conversation
turbolent
requested review from
janezpodhostnik,
peterargue,
m-Peter,
zhangchiqing and
ramtinms
as code owners
WalkthroughThe pull request includes updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🧰 Additional context used🔇 Additional comments (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
tests/web3js/eth_revert_reason_test.js (1)
7-7: Avoid unrelated style changes in dependency update PRs.The change from double quotes to single quotes is purely cosmetic and unrelated to the Cadence upgrade. Consider reverting this change to maintain focus on the dependency updates.
tests/integration_test.go (1)
197-201: Document the block height offset.While using
startingBlockHeightimproves maintainability, the+2offset in the assertions is not documented. Consider adding a comment explaining why we expect the block number to bestartingBlockHeight + 2to help future maintainers understand the test's assumptions.Example comment:
+ // We expect blockNumber to be startingBlockHeight + 2 because: + // 1. One block is created during emulator startup + // 2. Another block is created during COA resource deployment assert.Equal(t, uint64(startingBlockHeight+2), blockNumber)tests/web3js/eth_non_interactive_test.js (1)
Line range hint
211-215: Document the Flow-to-EVM transaction signature mapping scheme.The structured pattern in signature values across transactions suggests a deterministic mapping scheme from Flow to EVM transactions. Consider adding documentation explaining this scheme.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- tests/integration_test.go (2 hunks)
- tests/web3js/build_evm_state_test.js (4 hunks)
- tests/web3js/config.js (1 hunks)
- tests/web3js/eth_filter_endpoints_test.js (2 hunks)
- tests/web3js/eth_non_interactive_test.js (4 hunks)
- tests/web3js/eth_revert_reason_test.js (3 hunks)
🧰 Additional context used
🔇 Additional comments (14)
tests/web3js/config.js (2)
2-2: LGTM: String formatting changes are consistent with modern JS practices.The switch to single quotes for strings is a common JavaScript convention and improves consistency.
Also applies to: 6-6
8-9: Please verify the block height increment's compatibility.The
startBlockHeighthas been increased from2nto3n. While this change is reflected across test files, we should verify if this increment is specifically required by the Cadence v1.2.1 upgrade.Let's check for any related changes or requirements:
tests/web3js/eth_revert_reason_test.js (2)
80-80: LGTM! Block height calculation is correct.The change to
conf.startBlockHeight + 3ncorrectly accounts for:
- The initial block
- The contract deployment transaction
- The assert error transaction
- The custom error transaction
46-46: LGTM! Verify block height calculation.The change to use dynamic block height references makes the test more robust. However, please verify that the offset of
+2naccounts for:
- The initial block
- The contract deployment transaction
- The assert error transaction
✅ Verification successful
Let me gather more context about the test file to understand the transaction flow better.
After analyzing the test file content, I can now generate the final response since I have enough context to verify the block height calculation.
Block height calculation is correct and verified
The offset of
+2ninconf.startBlockHeight + 2nis accurate because the test performs exactly two transactions before this assertion:
- Contract deployment transaction (
helpers.deployContract('storage'))- Assert error transaction (
eth_sendRawTransactionwithassertErrorTx)This is further validated by the subsequent test section that uses
+3nafter executing one more transaction (the custom error test).🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the block height calculation logic in other test files rg -A 2 "startBlockHeight.*[+].*2n" tests/Length of output: 298
Script:
#!/bin/bash # Check the full test file content to understand the transaction sequence cat tests/web3js/eth_revert_reason_test.js # Also search for any other block number assertions in tests rg "block.number" tests/Length of output: 7019
tests/integration_test.go (1)
28-28: LGTM! Good constant extraction.The introduction of
startingBlockHeightconstant improves maintainability by removing magic numbers and centralizing the configuration of the starting block height.tests/web3js/build_evm_state_test.js (5)
9-10: LGTM: Improved test maintainability with dynamic block height tracking.Good change to initialize block height from configuration instead of hardcoding. This makes the test more maintainable and flexible across different environments.
41-43: LGTM: Well-documented block height calculation.Good addition of comments explaining the block height increment logic. The assertion correctly verifies the blockchain state after EOA transactions.
Also applies to: 49-49
92-94: LGTM: Clear documentation of block height progression.Good explanation of the 60-block increment resulting from 20 EOAs performing 3 transfers each. The assertion correctly validates the blockchain state.
Also applies to: 97-97
143-145: LGTM: Accurate block height tracking after contract operations.Good documentation of the 60-block increment resulting from contract deployments (20), store operations (20), and sum operations (20). The assertion correctly validates the final blockchain state.
Also applies to: 148-148
Line range hint
153-158: Verify historical block number references.The test uses hardcoded block numbers (82n) for historical state verification. Consider updating these to use dynamic block height calculations for consistency with the rest of the changes.
Consider refactoring the historical block number references to use calculated values:
- }, 82n) + }, expectedBlockHeight - 60n) // Calculate based on the known number of transactionsAlso applies to: 171-174, 182-185
tests/web3js/eth_non_interactive_test.js (3)
317-317: Verify block height changes across the codebase.The block height has been updated from 2 to 3. This change should be consistent with other test files and configurations.
#!/bin/bash # Search for other occurrences of block heights in test files rg -A 1 "startBlockHeight.*[23]" --type js tests/ rg "0x[23].*block" --type js tests/Also applies to: 330-330
206-210: Verify the non-standard ECDSA signature values.The transaction signature components have non-standard values:
v = 0xff(typical Ethereum values are 27/28)randshave very structured valuesPlease verify if these values are intentionally set this way for Flow-to-EVM transaction mapping.
381-383: Verify fee history changes with Cadence v1.2.1.The fee history response now includes an additional block entry. Please verify if this change is:
- Required by the Cadence v1.2.1 upgrade
- Related to the block height changes
- Consistent with the Flow emulator v1.1.0 behavior
tests/web3js/eth_filter_endpoints_test.js (1)
393-393: Block number updates align with configuration changes.The block number updates from '0xc' (12) to '0xd' (13) in the transaction assertions are consistent with the related changes in
config.jswherestartBlockHeightwas updated from2nto3n. This maintains the test's integrity with the broader configuration updates.Let's verify the block height configuration changes:
Also applies to: 413-413
Description
Automatically update to:
Summary by CodeRabbit