Local state using remote registers #562

sideninja · 2024-09-20T12:25:52Z

Closes: #574

Description

This PR adds state read and execution using remote registers API from the AN.

For contributor use:

Targeted PR against master branch
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the standards mentioned here.
Updated relevant documentation
Re-reviewed Files changed in the Github PR explorer
Added appropriate labels

Summary by CodeRabbit

Release Notes

New Features
- Introduced a BlockExecutor for executing transactions in the EVM context on the Flow blockchain.
- Enhanced caching mechanisms for register values and slab indices to improve performance.
- Added new test cases to validate cadence architecture functions and block environment functions.
Refactor
- Reorganized codebase by transitioning client types from requester to evm across multiple services.
- Simplified transaction execution logic and improved error handling in the EVMClient.
Bug Fixes
- Updated client types in the event subscription functionality to ensure proper integration with the new evm services.
- Adjusted expected gas metrics in various tests to reflect changes in contract deployment and interaction.
Chores
- Updated dependency versions in go.mod files to ensure compatibility with the latest changes.
- Removed specific entries from .gitignore for better file management.

coderabbitai

Actionable comments posted: 5

Outside diff range and nitpick comments (9)

tests/e2e_web3js_test.go (2)

31-35: Update the TODO comment and follow up on the progress.

It's great to see a new test case being added for "cadence arch and environment calls". However, the TODO comment indicates that this test is currently failing due to an issue with the cadence arch replayer.

Consider updating the TODO comment with more details about the specific issue or converting it to a GitHub issue for better tracking and visibility.

Also, please check with Ramtin on the progress of the fix and update the codebase accordingly once it's available.

159-159: Clarify the reason for increasing the gas limit.

The gas limit for the contract deployment transaction has been increased from 1_250_000 to 1_550_000. While increasing the gas limit may be necessary to accommodate the complexity of the contract being deployed, it would be helpful to understand the specific reason behind this change.

Please consider:

Adding a comment explaining the need for the higher gas limit.

Evaluating the trade-offs between allowing more computational work and the increased transaction costs.

Adjusting the gas limit based on the contract complexity and transaction cost constraints.

go.mod (1)

91-91: LGTM!

The addition of the indirect dependency github.com/hashicorp/golang-lru/v2 v2.0.7 should not cause any issues, as it is managed by the Go module system.

As a best practice, consider reviewing the release notes or changelog of the added dependency to ensure there are no known issues or vulnerabilities.

services/evm/client.go (1)

53-57: Address the TODO comment about extracting transaction submission to a separate type.

Consider creating a new interface, such as TransactionSubmitter, to handle transaction submission responsibilities. This will help separate concerns and improve the cohesion of the EVMClient interface.
tests/fixtures/storage.sol (1)
11-11: Nitpick: Explicitly specify uint256 instead of uint for consistency

In the event declaration for BlockTime, consider using uint256 instead of uint to maintain consistency with the other event declarations and improve code clarity.

Apply this diff:
-event BlockTime(address indexed caller, uint value);
+event BlockTime(address indexed caller, uint256 value);
tests/web3js/build_evm_state_test.js (1)

176-176: Review the necessity of extending the test timeout

A timeout of 180*1000 milliseconds (180 seconds) has been added to the test case. Please confirm that this extended duration is necessary for the test to complete and consider optimizing the test to reduce execution time if possible.
tests/web3js/cadence_arch_env_test.js (3)
7-13: Improve the clarity and grammar of the introductory comments

The comments in lines 7-13 contain grammatical errors and could be rephrased for better readability and understanding.

Consider updating the comments as follows:
-// this test calls different environment and cadenc arch functions. It uses view
-// function to call and return the value which it compares with the block on the network,
-// behind the scene these causes the local client to make such a call and return the value
-// and this test makes sure the on-chain data is same as local-index data. Secondly, this
-// test also submits a transaction that emits the result, which checks the local state
-// re-execution of transaction, and makes sure both receipt matches (remote and local),
-// this in turn tests the local state re-execution.
+// This test calls different environment and Cadence architecture functions. It uses view
+// functions to call and return values, which it compares with the blocks on the network.
+// Behind the scenes, this causes the local client to make such calls and return the values,
+// ensuring that the on-chain data is the same as the local index data. Secondly, this
+// test also submits a transaction that emits the result, which checks the local state
+// re-execution of the transaction, and makes sure both receipts (remote and local) match.
+// This, in turn, tests the local state re-execution.
18-18: Prefer const over let for variables that are not reassigned

In lines 18, 32, and 51, the variables res, block, and deployed are declared with let but are not reassigned. Using const can prevent accidental reassignment and improve code clarity.

Update the variable declarations:
-        let res = await helpers.signAndSend({
+        const res = await helpers.signAndSend({
-        let block = await web3.eth.getBlock('latest')
+        const block = await web3.eth.getBlock('latest')
-        let deployed = await helpers.deployContract('storage')
+        const deployed = await helpers.deployContract('storage')
Also applies to: 32-32, 51-51

60-62: Address the TODO regarding block environment functions

The TODO comment indicates an issue where Ethereum calls are executed at the provided block height, but block environment functions (number, hash, etc.) point to the next block (the block proposal). This discrepancy may affect test accuracy.

Would you like assistance in investigating this issue or opening a GitHub issue to track and resolve this problem?

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 6261e47 and 67f5825.

Files ignored due to path filters (2)

tests/go.sum is excluded by !**/*.sum
tests/web3js/package-lock.json is excluded by !**/package-lock.json

Files selected for processing (16)

bootstrap/bootstrap.go (7 hunks)
go.mod (1 hunks)
services/evm/client.go (13 hunks)
services/evm/executor.go (1 hunks)
tests/e2e_web3js_test.go (2 hunks)
tests/fixtures/storage.byte (1 hunks)
tests/fixtures/storage.sol (4 hunks)
tests/fixtures/storageABI.json (4 hunks)
tests/go.mod (1 hunks)
tests/helpers.go (3 hunks)
tests/web3js/build_evm_state_test.js (2 hunks)
tests/web3js/cadence_arch_env_test.js (1 hunks)
tests/web3js/eth_deploy_contract_and_interact_test.js (2 hunks)
tests/web3js/eth_filter_endpoints_test.js (2 hunks)
tests/web3js/eth_revert_reason_test.js (2 hunks)
tests/web3js/eth_transaction_type_fees_test.js (3 hunks)

Files skipped from review as they are similar to previous changes (2)

bootstrap/bootstrap.go
services/evm/executor.go

Additional comments not posted (53)

tests/web3js/eth_revert_reason_test.js (2)

44-44: LGTM!

The change simplifies the process of obtaining the reverted transaction by directly using the transaction hash, eliminating the need to fetch the latest block. This improves code efficiency.

74-74: Duplicate change.

This change is identical to the one at line 44. It simplifies the process of obtaining the reverted transaction by directly using the transaction hash, eliminating the need to fetch the latest block. This improves code efficiency.

tests/web3js/eth_transaction_type_fees_test.js (4)

24-24: LGTM!

The change updates the expected gas used value, which aligns with the AI-generated summary. The new value seems reasonable.

30-30: LGTM!

The change updates the expected sender balance, which aligns with the AI-generated summary. The new value seems reasonable.

75-75: LGTM!

The change updates the expected sender balance, which aligns with the AI-generated summary. The new value seems reasonable.

132-132: LGTM!

The change updates the expected sender balance, which aligns with the AI-generated summary. The new value seems reasonable.

tests/web3js/eth_deploy_contract_and_interact_test.js (2)

21-21: LGTM!

The change in the expected gas usage for the contract deployment process is valid and does not introduce any issues.

236-236: LGTM!

The change in the expected gas estimate for the contract interaction is valid and does not introduce any issues.

tests/fixtures/storageABI.json (16)

23-41: LGTM!

The BlockHash event is well-defined with clear input names and appropriate types. Indexing the caller input is a good practice for efficient event filtering.

42-60: LGTM!

The BlockNumber event is well-defined with clear input names and appropriate types. Indexing the caller input is a good practice for efficient event filtering.

61-79: LGTM!

The BlockTime event is well-defined with clear input names and appropriate types. Indexing the caller input is a good practice for efficient event filtering.

111-129: LGTM!

The ChainID event is well-defined with clear input names and appropriate types. Indexing the caller input is a good practice for efficient event filtering.

149-167: LGTM!

The Random event is well-defined with clear input names and appropriate types. Indexing the caller input is a good practice for efficient event filtering.

168-186: LGTM!

The Retrieved event is well-defined with clear input names and appropriate types. Indexing the caller input is a good practice for efficient event filtering.

187-205: LGTM!

The VerifyArchCallToFlowBlockHeight event is well-defined with clear input names and appropriate types. Indexing the caller input is a good practice for efficient event filtering.

206-224: LGTM!

The VerifyArchCallToRandomSource event is well-defined with clear input names and appropriate types. Indexing the caller input is a good practice for efficient event filtering.

225-243: LGTM!

The VerifyArchCallToRevertibleRandom event is well-defined with clear input names and appropriate types. Indexing the caller input is a good practice for efficient event filtering.

244-262: LGTM!

The VerifyArchCallToVerifyCOAOwnershipProof event is well-defined with clear input names and appropriate types. Indexing the caller input is a good practice for efficient event filtering.

355-367: LGTM!

The emitBlockHash function is well-defined with a clear parameter name and appropriate type. The nonpayable modifier is suitable for a function that emits an event without modifying the contract state.

368-374: LGTM!

The emitBlockNumber function is well-defined with a clear name. The nonpayable modifier is suitable for a function that emits an event without modifying the contract state.

375-381: LGTM!

The emitBlockTime function is well-defined with a clear name. The nonpayable modifier is suitable for a function that emits an event without modifying the contract state.

382-388: LGTM!

The emitChainID function is well-defined with a clear name. The nonpayable modifier is suitable for a function that emits an event without modifying the contract state.

389-395: LGTM!

The emitRandom function is well-defined with a clear name. The nonpayable modifier is suitable for a function that emits an event without modifying the contract state.

396-402: LGTM!

The emitRetrieve function is well-defined with a clear name. The nonpayable modifier is suitable for a function that emits an event without modifying the contract state.

tests/helpers.go (2)

65-68: LGTM!

The changes to the testLogWriter function look good. The function now creates a new zerolog.ConsoleWriter instance, sets the global time field format to time.RFC3339Nano, and configures the writer's time format to "04:05.0000" before returning the writer.

338-339: Looks good!

The change to the evmSign function signature, moving the data []byte parameter to a new line, improves readability without altering the function's behavior.

tests/go.mod (1)

156-156: Verify compatibility of the dependency update.

The github.com/onflow/flow/protobuf/go/flow dependency has been updated from version v0.4.6 to v0.4.7.

While minor version updates are generally safe, it's important to ensure that this update is compatible with the rest of the project dependencies and doesn't introduce any breaking changes or issues.

Run the following script to check if the update causes any issues:

tests/web3js/eth_filter_endpoints_test.js (2)

364-364: LGTM!

The updated transaction hash looks good and matches the expected format.

417-417: Looks good!

The updated hash and value fields in the expected transaction object are valid and match the Ethereum transaction format.

Also applies to: 422-422

services/evm/client.go (10)

93-102: LGTM!

The RemoteClient struct has the appropriate dependencies and fields to implement the EVMClient interface. The naming and organization of the fields are clear and follow Go conventions.

Line range hint 120-197: LGTM!

The NewEVM function properly initializes the RemoteClient instance with the provided dependencies. The checks for the COA account balance and the creation of the COA resource based on the configuration flag are useful safeguards. The function logic is clear and follows best practices.

Line range hint 201-259: LGTM!

The SendRawTransaction method follows the expected steps for sending a raw transaction. The validation of the transaction, the check for the minimum gas price, and the building of the Flow transaction are all good practices. The method logic is clear and well-structured. The logging of the transaction details provides good visibility into the process.

Line range hint 260-308: LGTM!

The buildTransaction method follows the necessary steps to build a valid Flow transaction. Retrieving the latest block and signer information concurrently using an error group is an efficient approach. Setting the transaction fields with the appropriate values and signing the transaction envelope with the configured signer ensures a properly constructed and authorized transaction. The error handling and formatting of error messages are appropriate.

310-321: LGTM!

The GetBalance method retrieves the balance of an address at a given EVM height using the stateAt method to get the state database. Retrieving the balance using the GetBalance method of the state database is the correct approach. The method returns the balance as a *big.Int, which is the expected type for balances in Go-Ethereum. The method logic is clear and follows best practices.

323-334: LGTM!

The GetNonce method retrieves the nonce of an address at a given EVM height using the stateAt method to get the state database. Retrieving the nonce using the GetNonce method of the state database is the correct approach. The method returns the nonce as a uint64, which is the expected type for nonces in Go-Ethereum. The method logic is clear and follows best practices.

Line range hint 336-348: LGTM!

The GetStorageAt method retrieves the storage value at a given address and hash at a specific EVM height using the stateAt method to get the state database. Retrieving the storage value using the GetState method of the state database is the correct approach. The method returns the storage value as a common.Hash, which is the expected type for storage values in Go-Ethereum. The method logic is clear and follows best practices.

350-357: LGTM!

The GetCode method retrieves the code at a given address at a specific EVM height using the stateAt method to get the state database. Retrieving the code using the GetCode method of the state database is the correct approach. The method returns the code as a byte slice, which is the expected type for code in Go-Ethereum. The method logic is clear and follows best practices.

359-379: LGTM!

The EstimateGas method estimates the gas required for a given transaction data at a specific EVM height using the executorAt method to get the block executor. Calling the Call method of the block executor to execute the transaction and get the gas consumed is the correct approach. The method handles the error cases and returns the appropriate error type based on the execution result, which is a good practice. The method logic is clear and follows best practices.

381-411: LGTM!

The Call method executes a given transaction data at a specific EVM height using the executorAt method to get the block executor. Calling the Call method of the block executor to execute the transaction and get the result is the correct approach. The method handles the error cases and returns the appropriate error type based on the execution result, which is a good practice. Ensuring that the returned data is an empty slice if it is nil is a good practice to match the behavior of the remote client. The method logic is clear and follows best practices.

tests/fixtures/storage.sol (10)

46-48: Function emitRetrieve correctly emits Retrieved event

The emitRetrieve function properly emits the Retrieved event with the caller's address and the stored number.

60-62: Function emitBlockNumber correctly emits BlockNumber event

The emitBlockNumber function properly emits the BlockNumber event with the caller's address and the current block number.

68-70: Function emitBlockTime correctly emits BlockTime event

The emitBlockTime function properly emits the BlockTime event with the caller's address and the current block timestamp.

84-86: Function emitRandom correctly emits Random event

The emitRandom function properly emits the Random event with the caller's address and the block.prevrandao value.

92-94: Function emitChainID correctly emits ChainID event

The emitChainID function properly emits the ChainID event with the caller's address and the current chain ID.

104-105: Function customError correctly uses a custom error

The customError function correctly demonstrates how to revert with the custom error MyCustomError.

115-118: Function emitVerifyArchCallToRandomSource correctly emits event

The emitVerifyArchCallToRandomSource function correctly calls verifyArchCallToRandomSource and emits the VerifyArchCallToRandomSource event with the caller's address and the output.

127-130: Function emitVerifyArchCallToRevertibleRandom correctly emits event

The emitVerifyArchCallToRevertibleRandom function correctly calls verifyArchCallToRevertibleRandom and emits the VerifyArchCallToRevertibleRandom event with the caller's address and the output.

139-142: Function emitVerifyArchCallToFlowBlockHeight correctly emits event

The emitVerifyArchCallToFlowBlockHeight function correctly calls verifyArchCallToFlowBlockHeight and emits the VerifyArchCallToFlowBlockHeight event with the caller's address and the output.

151-154: Function emitVerifyArchCallToVerifyCOAOwnershipProof correctly emits event

The emitVerifyArchCallToVerifyCOAOwnershipProof function correctly calls verifyArchCallToVerifyCOAOwnershipProof and emits the VerifyArchCallToVerifyCOAOwnershipProof event with the caller's address and the output.

tests/web3js/build_evm_state_test.js (2)

158-158: Confirm that the increased estimated gas value is expected

The estimated gas value has been updated to 29338n. Please verify that this increase is intentional and aligns with the expected changes in the contract or gas estimation logic.

165-165: Confirm that the increased contract code length is expected

The length of the contract code retrieved at the latest block has increased to 12180. This significant change suggests alterations in the contract's bytecode. Please ensure that this is expected and corresponds to the updates made.

tests/web3js/cadence_arch_env_test.js (2)

74-76: Address the TODO regarding block environment functions

The same issue regarding block environment functions pointing to the next block is mentioned here. It's important to resolve this to ensure test reliability.

90-92: Address the TODO regarding block environment functions

This TODO repeats the concern about block environment functions pointing to the next block instead of the provided block height. Resolving this will enhance the accuracy of your tests.

coderabbitai · 2024-09-20T13:57:58Z

tests/fixtures/storage.sol

+    function emitBlockHash(uint num) public {
+        emit BlockHash(msg.sender, blockhash(num));
+    }


Add input validation for num parameter in emitBlockHash

The blockhash function only returns meaningful values for the 256 most recent blocks. To prevent unexpected results, consider adding input validation to ensure that num is within the valid range (block.number - 256 <= num < block.number).

Apply this diff:

function emitBlockHash(uint num) public { + require(num >= block.number - 256 && num < block.number, "Invalid block number"); emit BlockHash(msg.sender, blockhash(num)); }

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

function emitBlockHash(uint num) public {

emit BlockHash(msg.sender, blockhash(num));

}

function emitBlockHash(uint num) public {

require(num >= block.number - 256 && num < block.number, "Invalid block number");

emit BlockHash(msg.sender, blockhash(num));

}

coderabbitai · 2024-09-20T13:57:59Z

tests/web3js/cadence_arch_env_test.js

+            web3.eth.abi.decodeParameter('uint', res.value).toString(),
+            (prev.timestamp+1n).toString(),  // investigate why timestamp is increasing by 1
+        )


Prevent TypeError due to mixing BigInt and Number types

The expression (prev.timestamp + 1n).toString() may cause a TypeError because prev.timestamp is a Number, and 1n is a BigInt.

Convert prev.timestamp to a BigInt before the addition:

- (prev.timestamp+1n).toString(), // investigate why timestamp is increasing by 1 + (BigInt(prev.timestamp) + 1n).toString(), // investigate why timestamp is increasing by 1

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

web3.eth.abi.decodeParameter('uint', res.value).toString(),

(prev.timestamp+1n).toString(), // investigate why timestamp is increasing by 1

)

web3.eth.abi.decodeParameter('uint', res.value).toString(),

(BigInt(prev.timestamp) + 1n).toString(), // investigate why timestamp is increasing by 1

)

coderabbitai · 2024-09-20T13:57:59Z

tests/web3js/cadence_arch_env_test.js

+        assert.equal(
+            web3.eth.abi.decodeParameter('uint256', res.value),
+            res.block.number+1n,
+        )


Prevent TypeError due to mixing BigInt and Number types

Adding a BigInt (1n) to a Number (res.block.number) can cause a TypeError in JavaScript. You cannot mix BigInt and Number types in arithmetic operations.

Convert res.block.number to a BigInt before performing the addition:

- res.block.number+1n, + BigInt(res.block.number) + 1n,

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

assert.equal(

web3.eth.abi.decodeParameter('uint256', res.value),

res.block.number+1n,

)

assert.equal(

web3.eth.abi.decodeParameter('uint256', res.value),

BigInt(res.block.number) + 1n,

)

coderabbitai · 2024-09-20T13:57:59Z

tests/web3js/cadence_arch_env_test.js

+        await testEmitTx(methods.emitRandom())
+
+        let res = await testCall(methods.random())
+        assert.isNotEmpty(web3.eth.abi.decodeParameter('uint256', res.value).toString())


Use a valid assertion method

The assert.isNotEmpty method may not exist in Chai's assert interface. To verify that the value is not empty, consider checking the length of the string.

Update the assertion as follows:

- assert.isNotEmpty(web3.eth.abi.decodeParameter('uint256', res.value).toString()) + assert.isAbove(web3.eth.abi.decodeParameter('uint256', res.value).toString().length, 0)

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

assert.isNotEmpty(web3.eth.abi.decodeParameter('uint256', res.value).toString())

assert.isAbove(web3.eth.abi.decodeParameter('uint256', res.value).toString().length, 0)

coderabbitai · 2024-09-20T13:57:59Z

tests/fixtures/storage.byte

Consider excluding generated bytecode from version control

Including compiled bytecode (storage.byte) in version control is generally discouraged. It can lead to larger repository sizes and potential merge conflicts when the bytecode changes. It's best practice to generate bytecode during the build or testing process.

Consider removing storage.byte from the repository and adding it to .gitignore to prevent future commits:

+ echo "tests/fixtures/storage.byte" >> .gitignore

Committable suggestion was skipped due to low confidence.

coderabbitai

Actionable comments posted: 0

Outside diff range and nitpick comments (1)

services/ingestion/engine_test.go (1)
553-559: LGTM! Consider extracting magic numbers.

The refactoring to use types.NewBlock constructor improves code readability and maintainability. It's a good change that simplifies the block creation process.

Consider extracting the magic numbers (1337 and 100) into named constants for better clarity and maintainability. For example:
+const (
+    defaultBlockNonce = uint64(1337)
+    defaultTotalSupply = 100
+)

 gethBlock := types.NewBlock(
     gethCommon.HexToHash("0x1"),
     height,
-    uint64(1337),
-    big.NewInt(100),
+    defaultBlockNonce,
+    big.NewInt(defaultTotalSupply),
     gethCommon.HexToHash("0x15"),
 )

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between bde1b52 and b1202f0.

Files selected for processing (5)

models/block.go (2 hunks)
models/block_test.go (2 hunks)
services/ingestion/engine_test.go (1 hunks)
tests/web3js/cadence_arch_env_test.js (1 hunks)
tests/web3js/eth_transaction_type_fees_test.js (4 hunks)

Files skipped from review as they are similar to previous changes (2)

tests/web3js/cadence_arch_env_test.js
tests/web3js/eth_transaction_type_fees_test.js

Additional comments not posted (4)

models/block.go (3)
78-81: New fixedHash variable introduced to handle specific case.

A new fixedHash variable has been introduced to handle cases where PrevRandao equals 0x0. This change seems to address a specific edge case or compatibility issue.

To understand the context and necessity of this change, please run the following script:
#!/bin/bash
# Search for references to PrevRandao and 0x0 hash
rg --type go "PrevRandao.*0x0|0x0.*PrevRandao"
This will help us understand if this is a common scenario in the codebase and if similar checks are performed elsewhere.

Also applies to: 95-95

Line range hint 75-95: Summary of changes in decodeBlockEvent function

The changes in this function appear to be part of a larger refactoring effort. Key modifications include:

Simplified error handling

Introduction of a fixedHash variable to handle specific cases

Removal of legacy block event decoding logic

While these changes simplify the codebase and address specific scenarios, they might affect backwards compatibility.

To ensure the safety of these changes, please run the following script to check for any tests that might be affected:
#!/bin/bash
# Search for tests related to block decoding
rg --type go "func.*Test.*Block.*Decod|func.*Test.*Decod.*Block" test
Review the results to ensure that existing tests still pass and consider adding new tests to cover the modified behavior, especially the new fixedHash logic.

75-76: Simplified error handling might affect backwards compatibility.

The error handling has been simplified to directly return the error from events.DecodeBlockEventPayload. While this makes the function more straightforward, it removes the attempt to decode a legacy block event.

To ensure this change doesn't break existing functionality, please run the following script to check for any other usages of legacy block event decoding in the codebase:

If there are no results, this change is likely safe. Otherwise, we may need to update those occurrences as well.
models/block_test.go (1)
Line range hint 153-163: Refactoring of block decoding test and stronger assertions

The changes in this test function improve its focus and add a crucial assertion:

The function decodeBlockEvent is now used instead of decodeLegacyBlockEvent. This suggests a consolidation of decoding logic, which is good for maintainability. However, ensure that this change is consistent across the codebase and that no legacy decoding functions are left unused.

The addition of require.NotNil(t, b.FixedHash) is a valuable assertion. It ensures that the decoded block has a non-nil FixedHash, which is important for the integrity of the block data.

The removal of the second decoding (b2) and its comparison simplifies the test, focusing on the correctness of a single decoding operation. This change makes the test more straightforward and easier to understand.

These changes appear to strengthen the test while simplifying its logic. However, to ensure completeness:

Let's verify if there are any remaining uses of decodeLegacyBlockEvent in the codebase:

Also, let's check if there are any other tests or functions that might need similar updates:

Verification successful

Verification Successful: decodeLegacyBlockEvent has been fully removed from the codebase. All references now use decodeBlockEvent.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Search for any remaining uses of decodeLegacyBlockEvent
rg "decodeLegacyBlockEvent" --type go
Length of output: 37

Script:
#!/bin/bash
# Search for other potential legacy decoding functions
rg "decode.*BlockEvent" --type go
Length of output: 485

coderabbitai · 2024-09-30T07:24:23Z

Walkthrough

The pull request introduces significant changes across multiple files, primarily focusing on restructuring the codebase to enhance the Ethereum Virtual Machine (EVM) integration with the Flow blockchain. Key modifications include updating client types, refining transaction execution logic, and implementing caching mechanisms for improved performance. Additionally, the .gitignore file has been updated, and various tests have been adjusted to align with the new functionalities and expectations.

Changes

Files	Change Summary
`.gitignore`	Removed entry for `tests/web3js/package-lock.json` and added `metrics/data/` to ignore list.
`api/api.go`, `bootstrap/bootstrap.go`	Updated client types from `requester` to `evm`, modifying struct fields and method signatures accordingly.
`go.mod`	Updated dependency versions for `github.com/onflow/flow-go` and `github.com/onflow/flow/protobuf/go/flow`.
`models/block.go`, `models/block_test.go`	Simplified block decoding logic and updated tests to reflect changes in decoding methods.
`services/evm/*.go`	Refactored EVM client interface, introduced new structs and methods for transaction execution and state management.
`tests/.go`, `tests/web3js/.js`	Adjusted test cases to align with new functionalities, updated expected values, and added new tests for coverage.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant EVMClient
    participant Ledger
    participant Emulator

    User->>EVMClient: Send Transaction
    EVMClient->>Ledger: Fetch State
    Ledger->>EVMClient: Return State
    EVMClient->>Emulator: Execute Transaction
    Emulator->>EVMClient: Return Execution Result
    EVMClient->>User: Return Transaction Receipt

Assessment against linked issues

Objective	Addressed	Explanation
Ensure scalability of remote register API during re-execution (574)	✅
Validate re-executed state (574)	❌	The changes do not include specific validation logic.

Possibly related issues

Local register index and state re-execution #322: The changes address the need for local state management, which is a key concern in the issue regarding local register indexing.

Suggested labels

Testing

Suggested reviewers

sideninja
peterargue

🐇 "In the code where changes bloom,
The EVM finds its room.
With clients new and tests refined,
A brighter path we now can find.
Hopping through each line with glee,
In Flow's embrace, we dance so free!" 🐇

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between a0e6820 and 0ff788b.

📒 Files selected for processing (3)

tests/e2e_web3js_test.go (2 hunks)
tests/helpers.go (4 hunks)
tests/web3js/cadence_arch_env_test.js (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (3)

tests/e2e_web3js_test.go
tests/helpers.go
tests/web3js/cadence_arch_env_test.js

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?

❤️ Share

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

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)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

coderabbitai

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (17)

services/evm/rotation_signer.go (3)
Line range hint 26-49: LGTM with a minor suggestion: Consider initializing the index field.

The constructor implementation is solid, with good compatibility checks between private keys and the hashing algorithm. However, consider explicitly initializing the index field to 0 in the struct initialization for clarity, even though it's the zero value for int.

Consider updating the struct initialization as follows:
 return &KeyRotationSigner{
 	keys:   keys,
 	hasher: hasher,
 	keyLen: len(keys),
+	index:  0,
 }, nil
Line range hint 63-72: Implementation looks good, but the TODO needs to be addressed.

The PublicKey method is correctly implemented with proper read locking for thread safety. However, the TODO comment highlights an important issue that needs to be resolved:
// todo make this function to wait if the transactions that used this public key
// is not yet executed on the network, this is so it prevents using
// the same public key for fetching key sequence number before the transaction
// that already used it is not executed and thus the key would be incremented.
This issue is crucial for maintaining the correct sequence of transactions and preventing potential conflicts.

Please implement the waiting mechanism described in the TODO comment to ensure proper transaction sequencing. This might involve:

Keeping track of pending transactions for each key.

Implementing a waiting mechanism that checks if a transaction has been executed before allowing the use of the same public key again.

Potentially using a condition variable or a channel-based approach for efficient waiting.

Consider opening a separate issue to track this task if it's not feasible to implement in this PR.

Line range hint 1-72: Overall, the implementation looks good with one important TODO to address.

The KeyRotationSigner implementation aligns well with the PR objectives, providing a thread-safe mechanism for signing with rotating keys. The code is well-structured and documented. The main point that needs attention is the TODO in the PublicKey method, which is crucial for maintaining correct transaction sequencing.

Consider the following suggestions for improving the overall design:

Implement the waiting mechanism described in the TODO comment of the PublicKey method.

Add a method to check if all pending transactions for a specific key have been executed.

Consider adding a configurable timeout for waiting on transaction execution to prevent indefinite blocking.

Implement error handling for cases where a key becomes unusable (e.g., network issues preventing transaction execution).

These improvements will enhance the robustness and reliability of the key rotation mechanism in high-concurrency scenarios.
services/evm/kms_key_rotation_signer.go (3)
Line range hint 35-73: LGTM: Well-implemented initialization function with proper error handling.

The NewKMSKeyRotationSigner function is well-structured and correctly initializes the KMSKeyRotationSigner struct. It properly handles errors at various stages of the initialization process and includes helpful logging.

Consider wrapping the error returned from kmsClient.SignerForKey to provide more context:
 kmsSigner, err := kmsClient.SignerForKey(ctx, key)
 if err != nil {
-    return nil, fmt.Errorf(
-        "could not create KMS signer for the key with ID: %s: %w",
-        key.KeyID,
-        err,
-    )
+    return nil, fmt.Errorf("could not create KMS signer for the key with ID %s: %w", key.KeyID, err)
 }
This change makes the error message more concise while still providing all necessary information.

Line range hint 75-106: LGTM: Thread-safe implementation with proper key rotation and performance logging.

The Sign method is well-implemented, ensuring thread-safety through the use of a mutex. The key rotation logic is correct, and the inclusion of performance logging is a good practice for monitoring.

Consider simplifying the error handling:
 signature, err := signer.Sign(message)
 if err != nil {
-    return nil, fmt.Errorf(
-        "failed to sign message with public key %s: %w",
-        signer.PublicKey(),
-        err,
-    )
+    return nil, fmt.Errorf("failed to sign message with public key %s: %w", signer.PublicKey(), err)
 }

-return signature, err
+return signature, nil
This change makes the error handling more concise and explicitly returns nil for the error when signing is successful.

Line range hint 1-122: Summary: Well-implemented KMSKeyRotationSigner supporting PR objectives

The KMSKeyRotationSigner implementation in this file aligns well with the PR objectives of implementing state reading and execution through the remote registers API. Key features include:

Efficient key rotation mechanism for optimizing transaction submissions.

Thread-safe implementation of signing and public key retrieval methods.

Proper error handling and informative logging throughout the code.

The code quality is high, with comprehensive documentation and adherence to best practices. This implementation should contribute positively to the goal of local re-execution of transactions using the flow-go EVM Emulator.

To address the scalability concerns mentioned in the linked issue #574, consider the following:

Implement a caching mechanism for frequently used register values to reduce API requests.

Add configurable timeouts for KMS operations to prevent potential bottlenecks.

Consider implementing a batching mechanism for signing operations to optimize performance under high load.

These enhancements could help mitigate potential performance issues related to register API requests during re-execution.
services/evm/cross-spork_client_test.go (2)
Line range hint 36-71: Test_CrossSporkClients is good, but could be enhanced.

The Test_CrossSporkClients function effectively tests the add and continuous methods of the sporkClients struct. It covers important scenarios such as:

Adding clients in non-sequential order

Retrieving clients based on various heights

Checking continuity of ranges

However, we could enhance the test coverage with additional scenarios:

Consider adding the following test cases:

Attempt to add overlapping ranges and verify the behavior.

Test the behavior when adding a client with a range that exactly touches another (e.g., 101-200 and 201-300).

Verify the behavior when attempting to retrieve a client at the exact boundary between two ranges.

Here's a sample implementation for the first suggestion:
t.Run("add overlapping ranges", func(t *testing.T) {
    clients := &sporkClients{}

    client1 := testutils.SetupClientForRange(10, 100)
    client2 := testutils.SetupClientForRange(50, 150)

    require.NoError(t, clients.add(client1))
    err := clients.add(client2)
    require.Error(t, err)
    require.Contains(t, err.Error(), "overlapping range")
})
Line range hint 73-134: Test_CrossSpork is comprehensive but could be more structured.

The Test_CrossSpork function covers a wide range of functionality for the CrossSporkClient, including:

Client retrieval for different heights

Past spork checks

Error handling for out-of-range heights

Various API method behaviors

While the coverage is good, we can improve the test structure and clarity:

Consider the following improvements:

Split the large test into smaller, focused subtests. This will improve readability and make it easier to identify which specific functionality is failing if a test breaks.

Use table-driven tests for similar checks (e.g., out-of-range error checks).

Add more descriptive error messages to assertions.

Here's an example of how you could refactor a portion of the test:
func Test_CrossSpork(t *testing.T) {
    // Setup code...

    t.Run("getClientForHeight", func(t *testing.T) {
        testCases := []struct {
            name          string
            height        uint64
            expectedClient access.Client
            expectedError  error
        }{
            {"past1 client", 150, past1, nil},
            {"past2 client", past2Last - 1, past2, nil},
            {"current client", 600, current, nil},
            {"out of range", 10, nil, errs.ErrHeightOutOfRange},
        }

        for _, tc := range testCases {
            t.Run(tc.name, func(t *testing.T) {
                c, err := client.getClientForHeight(tc.height)
                if tc.expectedError != nil {
                    require.ErrorIs(t, err, tc.expectedError)
                } else {
                    require.NoError(t, err)
                    require.Equal(t, tc.expectedClient, c)
                }
            })
        }
    })

    // More subtests...
}
This refactoring improves readability and makes it easier to add new test cases in the future.
tests/fixtures/storage.sol (5)
42-48: LGTM: Good addition of emitRetrieve function

The new emitRetrieve function is a good addition, allowing event-based tracking of retrieve calls without modifying the original function. This separation of concerns is a good practice.

Consider adding a view modifier to emitRetrieve since it doesn't modify the contract state:
-    function emitRetrieve() public {
+    function emitRetrieve() public view {
         emit Retrieved(msg.sender, number);
     }
100-105: LGTM: Error handling functions added

The new assertError and customError functions are good additions for demonstrating different error handling mechanisms in Solidity. The use of a custom error in customError is particularly good for gas efficiency.

Consider adding comments to explain the purpose of these functions, especially if they are intended for testing or educational purposes:
+    // Function to demonstrate a simple assert error
     function assertError() public pure {
         require(false, "Assert Error Message");
     }

+    // Function to demonstrate a custom error with parameters
     function customError() public pure {
         revert MyCustomError(5, "Value is too low");
     }
115-118: LGTM: emitVerifyArchCallToRandomSource function added

The new emitVerifyArchCallToRandomSource function is a good addition, allowing event-based tracking of random source verifications. This can be useful for auditing and monitoring purposes.

Consider adding a comment to explain the purpose of the height parameter:
-    function emitVerifyArchCallToRandomSource(uint64 height) public {
+    // Emits the result of verifying the random source for a specific block height
+    function emitVerifyArchCallToRandomSource(uint64 height) public {
         bytes32 output = verifyArchCallToRandomSource(height);
         emit VerifyArchCallToRandomSource(msg.sender, output);
     }
127-142: LGTM: Verification emit functions added

The new emitVerifyArchCallToRevertibleRandom and emitVerifyArchCallToFlowBlockHeight functions are good additions, allowing event-based tracking of their respective verifications. This enhances the contract's ability to provide off-chain visibility into these operations.

Consider adding comments to explain the purpose of these functions:
+    // Emits the result of verifying a revertible random number
     function emitVerifyArchCallToRevertibleRandom() public {
         uint64 output = verifyArchCallToRevertibleRandom();
         emit VerifyArchCallToRevertibleRandom(msg.sender, output);
     }

+    // Emits the current Flow block height
     function emitVerifyArchCallToFlowBlockHeight() public {
         uint64 output = verifyArchCallToFlowBlockHeight();
         emit VerifyArchCallToFlowBlockHeight(msg.sender, output);
     }
144-154: LGTM with suggestions: emitVerifyArchCallToVerifyCOAOwnershipProof function added

The new emitVerifyArchCallToVerifyCOAOwnershipProof function is a good addition, allowing event-based tracking of COA ownership proof verifications. This can be useful for auditing and monitoring purposes.

Consider the following improvements for clarity and documentation:

Add comments to explain the purpose of the function and its parameters:
+    // Verifies and emits the result of a COA ownership proof
+    // @param arg0 The address to verify ownership for
+    // @param arg1 The hash of the ownership proof
+    // @param arg2 The ownership proof data
     function emitVerifyArchCallToVerifyCOAOwnershipProof(address arg0, bytes32 arg1, bytes memory arg2) public {
         bool output = verifyArchCallToVerifyCOAOwnershipProof(arg0, arg1, arg2);
         emit VerifyArchCallToVerifyCOAOwnershipProof(msg.sender, output);
     }
Consider using more descriptive parameter names to improve readability:
-    function emitVerifyArchCallToVerifyCOAOwnershipProof(address arg0, bytes32 arg1, bytes memory arg2) public {
+    function emitVerifyArchCallToVerifyCOAOwnershipProof(address ownerAddress, bytes32 proofHash, bytes memory proofData) public {
-        bool output = verifyArchCallToVerifyCOAOwnershipProof(arg0, arg1, arg2);
+        bool output = verifyArchCallToVerifyCOAOwnershipProof(ownerAddress, proofHash, proofData);
         emit VerifyArchCallToVerifyCOAOwnershipProof(msg.sender, output);
     }
services/evm/cross-spork_client.go (4)
Line range hint 29-83: LGTM: Well-implemented sporkClients type and methods, with a suggestion for error handling.

The sporkClients type and its methods (add, get, and continuous) are well-implemented and serve their purposes effectively. The sorting of clients and the continuity check are particularly important for maintaining the integrity of the spork client list.

However, consider improving error handling in the add method:

In the add method, consider wrapping the errors returned from GetLatestBlockHeader and GetNodeVersionInfo with more context, perhaps including the client details.

You might want to add error checking after the append operation to ensure it was successful.

Example:
if err := client.GetLatestBlockHeader(context.Background(), true); err != nil {
    return fmt.Errorf("failed to get latest block header for client %v: %w", client, err)
}
Line range hint 103-137: Improve the temporary fix for the Emulator.

The current implementation has a temporary fix for the Emulator, which doesn't support the GetNodeVersionInfo method. Consider the following improvements:

Add a TODO comment explaining why this fix is temporary and what the long-term solution should be.

Instead of checking the chain ID, consider adding a method to the access.Client interface that checks if GetNodeVersionInfo is supported, and use that check instead.

Example:
// TODO: This is a temporary fix. Long-term solution should involve updating the Emulator to support GetNodeVersionInfo.
if supportsNodeVersionInfo(currentSpork) {
    info, err := currentSpork.GetNodeVersionInfo(context.Background())
    if err != nil {
        return nil, fmt.Errorf("failed to get node version info: %w", err)
    }
    nodeRootBlockHeight = info.NodeRootBlockHeight
}

// Add this helper function
func supportsNodeVersionInfo(client access.Client) bool {
    _, err := client.GetNodeVersionInfo(context.Background())
    return err == nil
}
Line range hint 139-151: Add more detailed logging in CrossSporkClient methods.

While the current implementation includes some logging, consider adding more detailed logs to aid in debugging and monitoring. For example:

In getClientForHeight, log when falling back to the current spork client.

In GetLatestHeightForSpork, log the retrieved latest height.

In ExecuteScriptAtBlockHeight, log the script execution (maybe hash of the script for privacy).

Example for getClientForHeight:
if !c.IsPastSpork(height) {
    c.logger.Debug().
        Uint64("requested-height", height).
        Msg("using current spork client")
    return c.Client, nil
}
Also applies to: 153-167, 169-179, 181-191, 193-203

Multiple Usages of Experimental Slices Package Identified

The golang.org/x/exp/slices package is imported in the following files:

services/evm/cross-spork_client.go

services/logs/filter.go

Using an experimental package across multiple modules can lead to potential instability if the API changes in future updates. Please ensure that this dependency is well-documented and monitor the package for any changes or updates.

🔗 Analysis chain

Line range hint 3-14: Consider the implications of using the experimental slices package.

The code imports golang.org/x/exp/slices, which is an experimental package. While it provides useful functionality, be aware that its API may change in the future. Consider documenting this dependency and keeping track of any updates to the package.
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Check if there are any other usages of the experimental slices package in the codebase
rg --type go 'golang\.org/x/exp/slices'
Length of output: 259

Script:
#!/bin/bash
# Check if there are any other usages of the experimental slices package in the codebase
rg --type go 'golang\.org/x/exp/slices'
Length of output: 154

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between e3e44ae and a0e6820.

⛔ Files ignored due to path filters (3)

go.sum is excluded by !**/*.sum
tests/go.sum is excluded by !**/*.sum
tests/web3js/package-lock.json is excluded by !**/package-lock.json

📒 Files selected for processing (32)

.gitignore (0 hunks)
api/api.go (3 hunks)
bootstrap/bootstrap.go (7 hunks)
go.mod (2 hunks)
models/block.go (2 hunks)
models/block_test.go (2 hunks)
services/evm/client.go (13 hunks)
services/evm/cross-spork_client.go (1 hunks)
services/evm/cross-spork_client_test.go (1 hunks)
services/evm/executor.go (1 hunks)
services/evm/kms_key_rotation_signer.go (1 hunks)
services/evm/pool.go (1 hunks)
services/evm/remote_ledger.go (3 hunks)
services/evm/remote_ledger_test.go (1 hunks)
services/evm/rotation_signer.go (1 hunks)
services/evm/rotation_signer_test.go (1 hunks)
services/ingestion/engine_test.go (1 hunks)
services/ingestion/subscriber.go (3 hunks)
services/ingestion/subscriber_test.go (6 hunks)
services/requester/requester_test.go (0 hunks)
tests/e2e_web3js_test.go (2 hunks)
tests/fixtures/storage.byte (1 hunks)
tests/fixtures/storage.sol (4 hunks)
tests/fixtures/storageABI.json (4 hunks)
tests/go.mod (1 hunks)
tests/helpers.go (3 hunks)
tests/web3js/build_evm_state_test.js (2 hunks)
tests/web3js/cadence_arch_env_test.js (1 hunks)
tests/web3js/eth_deploy_contract_and_interact_test.js (2 hunks)
tests/web3js/eth_filter_endpoints_test.js (2 hunks)
tests/web3js/eth_revert_reason_test.js (2 hunks)
tests/web3js/eth_transaction_type_fees_test.js (4 hunks)

💤 Files with no reviewable changes (2)

.gitignore
services/requester/requester_test.go

✅ Files skipped from review due to trivial changes (1)

tests/go.mod

🚧 Files skipped from review as they are similar to previous changes (24)

api/api.go
bootstrap/bootstrap.go
go.mod
models/block.go
models/block_test.go
services/evm/client.go
services/evm/executor.go
services/evm/pool.go
services/evm/remote_ledger.go
services/evm/remote_ledger_test.go
services/evm/rotation_signer_test.go
services/ingestion/engine_test.go
services/ingestion/subscriber.go
services/ingestion/subscriber_test.go
tests/e2e_web3js_test.go
tests/fixtures/storage.byte
tests/fixtures/storageABI.json
tests/helpers.go
tests/web3js/build_evm_state_test.js
tests/web3js/cadence_arch_env_test.js
tests/web3js/eth_deploy_contract_and_interact_test.js
tests/web3js/eth_filter_endpoints_test.js
tests/web3js/eth_revert_reason_test.js
tests/web3js/eth_transaction_type_fees_test.js

🔇 Additional comments (19)

services/evm/rotation_signer.go (3)

Line range hint 1-7: LGTM: Package change and imports are appropriate.

The package change from requester to evm aligns with the PR objectives. The imports are relevant for the implemented functionality.

Line range hint 9-24: LGTM: Well-designed struct with clear documentation.

The KeyRotationSigner struct is well-defined with appropriate fields. The documentation clearly explains its purpose and behavior, including its concurrency safety. The implementation of the crypto.Signer interface is correctly indicated.

Line range hint 51-61: LGTM: Thread-safe implementation with correct key rotation.

The Sign method is well-implemented with proper mutex usage for thread safety. The signing process and key rotation logic are correctly implemented.

services/evm/kms_key_rotation_signer.go (3)

Line range hint 1-11: LGTM: Package declaration and imports are appropriate.

The package name change from 'requester' to 'evm' aligns well with the file's location in the 'evm' directory. The imports cover necessary standard library packages and external dependencies required for the implementation.

Line range hint 13-33: LGTM: Well-designed struct with comprehensive documentation.

The KMSKeyRotationSigner struct is well-defined with appropriate fields for managing a pool of KMS signers. The documentation clearly explains its purpose, functionality, and benefits, including its concurrency-safety and optimization for transaction submissions. The implementation of the crypto.Signer interface is appropriate for the intended use case.

Line range hint 108-122: LGTM: Thread-safe implementation with potential for future improvement.

The PublicKey method is correctly implemented and ensures thread-safety by using a read lock.

Regarding the TODO comment, it suggests an important improvement for key sequence management. To track this, consider creating an issue in the project's issue tracker. Here's a script to check if an issue already exists for this TODO:

If no relevant issue is found, please create one to track this improvement.
services/evm/cross-spork_client_test.go (3)
Line range hint 17-34: Test_CrossSporkClient looks good!

The test function Test_CrossSporkClient is well-structured and comprehensive. It covers various scenarios including:

Edge cases (first and last heights)

Values within the range

Values outside the range (both below and above)

This ensures that the contains method of the sporkClient struct is working correctly for all possible inputs.

Line range hint 1-134: Overall, the test suite is comprehensive but could benefit from some structural improvements.

The test suite for the CrossSporkClient implementation covers a wide range of functionality and scenarios. However, there are a few areas where we can enhance the tests:

In Test_CrossSporkClients, consider adding more edge cases, such as overlapping ranges and boundary conditions.

For Test_CrossSpork, refactor the large test into smaller, focused subtests to improve readability and maintainability.

Implement table-driven tests where appropriate to make it easier to add new test cases in the future.

These improvements will make the test suite more robust, easier to maintain, and simpler to extend as the CrossSporkClient functionality evolves.

1-1: Package name change looks good, but let's verify consistency.

The package name change from requester to evm aligns well with the file path. This change improves the organization of the codebase.

Let's verify that this change is consistent across the codebase:

✅ Verification successful

Package name change is consistent across the codebase.

The package name has been successfully changed from requester to evm in all relevant files, ensuring consistency and enhancing the organization of the codebase.
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Check for any remaining references to the old package name and verify the new package name usage.

# Test 1: Check for any remaining "package requester" declarations
echo "Checking for any remaining 'package requester' declarations:"
rg --type go "^package requester$"

# Test 2: Verify the usage of "package evm" in the evm directory
echo "Verifying 'package evm' usage in the evm directory:"
rg --type go "^package evm$" services/evm/
Length of output: 775
tests/fixtures/storage.sol (5)

9-18: LGTM: New events are well-structured

The newly added events are well-defined and follow Solidity best practices. They provide a comprehensive way to log various contract interactions and state changes, which will be useful for off-chain monitoring and analysis.

60-62: LGTM: emitBlockNumber function added

The new emitBlockNumber function is a good addition, allowing event-based tracking of block numbers. This can be useful for various off-chain applications that need to monitor block progression.

68-70: LGTM: emitBlockTime function added

The new emitBlockTime function is a good addition, allowing event-based tracking of block timestamps. This can be useful for time-sensitive off-chain applications that need to monitor block times.

72-78: LGTM with suggestion: emitBlockHash function added

The new emitBlockHash function is a good addition, allowing event-based tracking of block hashes.

Please note that the previous review comment about adding input validation for the num parameter is still applicable. Consider implementing the suggested input validation to ensure num is within the valid range (block.number - 256 <= num < block.number).

92-94: LGTM: emitChainID function added

The new emitChainID function is a good addition, allowing event-based tracking of the chain ID. This can be useful for multi-chain applications or for verifying the correct network context.
services/evm/cross-spork_client.go (5)
1-1: LGTM: Package name change aligns with PR objectives.

The package name change from requester to evm better reflects the file's content and purpose, aligning with the PR's goal of implementing state reading and execution through the remote registers API.

Line range hint 16-27: LGTM: Well-defined sporkClient struct and method.

The sporkClient struct and its contains method are well-implemented. The struct efficiently stores the necessary information for each spork client, and the contains method correctly checks if a given height is within the spork's range.

Line range hint 85-101: LGTM: Well-designed CrossSporkClient struct.

The CrossSporkClient struct is well-designed to handle cross-spork functionality. It effectively wraps the Flow AN client and provides methods to access different AN APIs based on height boundaries of the sporks.

Line range hint 1-203: Overall assessment: Well-implemented CrossSporkClient with minor suggestions for improvement.

The implementation of the CrossSporkClient in this file effectively addresses the PR objectives of implementing state reading and execution through the remote registers API from the Application Node. The code is well-structured and provides a solid foundation for handling cross-spork functionality.

Key points:

The package name change and new imports align well with the PR goals.

The sporkClient and sporkClients types are well-designed and implemented.

The CrossSporkClient struct and its methods effectively manage multiple spork clients and delegate calls based on height.

Suggestions for improvement:

Consider the implications of using the experimental slices package.

Improve error handling in the sporkClients.add method.

Refine the temporary fix for the Emulator in NewCrossSporkClient.

Add more detailed logging in some methods to aid debugging and monitoring.

Verify the implementation of GetLatestHeightForSpork to ensure it always returns a height within the correct spork's range.

While this implementation provides a solid foundation, additional testing may be required to address the scalability concerns mentioned in the linked issue, particularly regarding the speed of register API requests during re-execution.

Line range hint 169-179: Verify the implementation of GetLatestHeightForSpork.

The GetLatestHeightForSpork method retrieves the latest height for a given spork. However, it's important to ensure that this method always returns a height within the correct spork's range.
#!/bin/bash
# Check if there are any tests for GetLatestHeightForSpork
rg --type go 'func.*Test.*GetLatestHeightForSpork'

# If no tests are found, output a message
if [ $? -ne 0 ]; then
    echo "No tests found for GetLatestHeightForSpork. Consider adding tests to verify its behavior."
fi

coderabbitai · 2024-09-30T07:31:56Z

tests/fixtures/storage.sol

+    function emitRandom() public {
+        emit Random(msg.sender, block.prevrandao);
+    }


💡 Codebase verification

Security Issue: Insecure Randomness Source in random Function

The random function relies on block.prevrandao, which can be manipulated by miners and is not suitable for critical operations requiring secure randomness.

File: tests/fixtures/storage.sol

Function: random()

Recommendation: Use a more secure randomness source, such as Chainlink VRF, to ensure unpredictability and security in critical operations.

🔗 Analysis chain

LGTM with security consideration: emitRandom function added

The new emitRandom function is a good addition, allowing event-based tracking of the block's prevrandao value.

Please ensure that this function is not used as the sole source of randomness for critical operations, as it can be manipulated by miners to some extent. Consider using a more secure randomness source for sensitive applications.

To verify the usage of this function, you can run the following script:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash # Description: Check for potentially unsafe usage of emitRandom or random functions # Test: Search for function calls that might be using emitRandom or random for sensitive operations rg --type solidity -e "emitRandom|random" -C 5

Length of output: 81

Script:

#!/bin/bash # Description: Check for potentially unsafe usage of emitRandom or random functions # Test: Search for function calls that might be using emitRandom or random for sensitive operations in Solidity files rg -g '*.sol' -e "emitRandom|random" -C 5

Length of output: 826

janezpodhostnik · 2024-10-02T16:01:56Z

models/block.go

+	if payload.PrevRandao == gethCommon.HexToHash("0x0") {
+		hash := payload.Hash.String()
+		fixedHash = &hash
 	}


@m-Peter @ramtinms I don't understand this part. Can you help me out?

@janezpodhostnik This is a part which is necessary for calculating the hash of blocks, when there are changes to its format (e.g. addition/removal of fields). The hash calculation can only be made on demand, with a function (https://github.com/onflow/flow-go/blob/master/fvm/evm/types/block.go#L54-L63). When testnet was launched, the block format had certain fields. After some time, we made an HCU to introduce a new field, PrevRandao. This naturally caused the Hash function to return different values for past blocks. And this part is basically an approach to mitigate that. We fix the block hash, to the value that was emitted in the EVM.BlockExecuted event.

sideninja requested review from peterargue and m-Peter as code owners September 20, 2024 12:25

This comment was marked as outdated.

Sign in to view

coderabbitai bot reviewed Sep 20, 2024

View reviewed changes

coderabbitai bot reviewed Sep 24, 2024

View reviewed changes

m-Peter force-pushed the feature/remote-state branch from 68d27a8 to 8ccf6d5 Compare September 24, 2024 09:58

m-Peter self-assigned this Sep 30, 2024

m-Peter added Improvement EVM labels Sep 30, 2024

m-Peter added this to the Flow-EVM-M2 milestone Sep 30, 2024

onflow deleted a comment from coderabbitai bot Sep 30, 2024

sideninja and others added 19 commits September 30, 2024 10:19

rename client

84f037a

rename package

f38cfc4

add block executor

ac580f2

modify requester to use local registers

d0b46d4

remove cache tester

e79aa53

improve remote ledger to support changes

f9bd0aa

renamed client from requester

65a54de

add block to executor

b162d07

tidy

0179ee6

change evm state test

8267e4b

update all tests to match the ones from local state index

3727afe

put precompiles back

9457eab

update run

3fbd8fb

add todo

a89f228

add todo

3a3fc9d

update comments

f325ce3

Update logic for decoding legacy EVM Block events

697ef57

Update test failures for E2E tests

eb8a879

Include package-lock.json file in git history

a0e6820

m-Peter force-pushed the feature/remote-state branch from 8ccf6d5 to a0e6820 Compare September 30, 2024 07:22

coderabbitai bot reviewed Sep 30, 2024

View reviewed changes

m-Peter added 2 commits September 30, 2024 10:55

Improve failure detection for E2E web3.js tests

ee70be0

Enable E2E tests for Cadence Arch

0ff788b

janezpodhostnik reviewed Oct 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Local state using remote registers #562

Local state using remote registers #562

sideninja commented Sep 20, 2024 •

edited by coderabbitai bot

Loading

This comment was marked as outdated.

coderabbitai bot left a comment

coderabbitai bot Sep 20, 2024

coderabbitai bot Sep 20, 2024

coderabbitai bot Sep 20, 2024

coderabbitai bot Sep 20, 2024

coderabbitai bot Sep 20, 2024

coderabbitai bot left a comment

coderabbitai bot commented Sep 30, 2024 •

edited

Loading

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

coderabbitai bot left a comment

coderabbitai bot Sep 30, 2024

janezpodhostnik Oct 2, 2024

m-Peter Oct 3, 2024

	assert.isNotEmpty(web3.eth.abi.decodeParameter('uint256', res.value).toString())
	assert.isAbove(web3.eth.abi.decodeParameter('uint256', res.value).toString().length, 0)

Local state using remote registers #562

Are you sure you want to change the base?

Local state using remote registers #562

Conversation

sideninja commented Sep 20, 2024 • edited by coderabbitai bot Loading

Description

Summary by CodeRabbit

Release Notes

This comment was marked as outdated.

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot Sep 20, 2024

Choose a reason for hiding this comment

coderabbitai bot Sep 20, 2024

Choose a reason for hiding this comment

coderabbitai bot Sep 20, 2024

Choose a reason for hiding this comment

coderabbitai bot Sep 20, 2024

Choose a reason for hiding this comment

coderabbitai bot Sep 20, 2024

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot commented Sep 30, 2024 • edited Loading

Walkthrough

Changes

Sequence Diagram(s)

Assessment against linked issues

Possibly related issues

Suggested labels

Suggested reviewers

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot Sep 30, 2024

Choose a reason for hiding this comment

janezpodhostnik Oct 2, 2024

Choose a reason for hiding this comment

m-Peter Oct 3, 2024

Choose a reason for hiding this comment

sideninja commented Sep 20, 2024 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Sep 30, 2024 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)