[Flow EVM] Add functionality to pause EVM transactions#8334
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a governance-controlled EVM pause flag ( Changes
Sequence Diagram(s)sequenceDiagram
participant User as User Tx
participant EVM as EVM Contract
participant Storage as Storage (/storage/evmOperationsPaused)
participant Internal as InternalEVM
User->>EVM: submit tx (deposit/run/deploy/call/withdraw)
EVM->>Storage: read evmOperationsPaused
alt paused == true
EVM-->>User: error "EVM operations are temporarily paused"
else paused == false
EVM->>Internal: delegate operation
Internal-->>EVM: result
EVM-->>User: success / result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
m-Peter
requested review from
holyfuchs,
janezpodhostnik,
peterargue,
turbolent and
zhangchiqing
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @fvm/evm/stdlib/contract.cdc:
- Around line 1067-1074: The isPaused() function currently dereferences the
result of self.account.storage.borrow<&Bool>(from: /storage/evmOperationsPaused)
which can be nil if nothing was stored; update isPaused to check that
evmOperationsPaused is non-nil before dereferencing and return false when it is
nil (i.e., use a nil-check or optional unwrapping around the evmOperationsPaused
borrow in isPaused to only dereference when present and otherwise return false).
🧹 Nitpick comments (2)
fvm/evm/evm_test.go (2)
4518-4537: Unused variablestoredValuesin batchRun test.The
storedValuesslice is populated but never used in the test assertions. Consider removing it to clean up the test code.♻️ Suggested cleanup
batchCount := 5 - var storedValues []int64 txBytes := make([]cadence.Value, batchCount) for i := 0; i < batchCount; i++ { num := int64(i) - storedValues = append(storedValues, num) // prepare batch of transaction payloads tx := testAccount.PrepareSignAndEncodeTx(t,
4330-4331: Consider adding a test for resuming EVM operations.The current tests verify that operations fail when paused, but there's no test verifying that operations succeed after the pause is removed (by setting
/storage/evmOperationsPausedtofalseor removing it). Adding such a test would provide more complete coverage of the pause/resume lifecycle.Would you like me to help generate a test case that verifies EVM operations resume correctly after unpausing?
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
engine/execution/state/bootstrap/bootstrap_test.gofvm/evm/evm_test.gofvm/evm/stdlib/contract.cdcutils/unittest/execution_state.go
🧰 Additional context used
📓 Path-based instructions (7)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/coding_conventions.mdc)
Follow Go coding conventions as documented in @docs/agents/CodingConventions.md
Follow Go coding standards and conventions as documented in @docs/agents/GoDocs.md
**/*.go: Follow the existing module structure in/module/,/engine/,/model/and use dependency injection patterns for component composition
Implement proper interfaces before concrete types
Follow Go naming conventions and the project's coding style defined in /docs/CodingConventions.md
Use mock generators: runmake generate-mocksafter interface changes
All inputs must be considered potentially byzantine; error classification is context-dependent and no code path is safe unless explicitly proven and documented
Use comprehensive error wrapping for debugging; avoidfmt.Errorf, useirrecoverablepackage for exceptions
NEVER log and continue on best effort basis; ALWAYS explicitly handle errors
Uses golangci-lint with custom configurations (.golangci.yml) and custom linters for Flow-specific conventions (struct write checking)
Files:
engine/execution/state/bootstrap/bootstrap_test.gofvm/evm/evm_test.goutils/unittest/execution_state.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Unit tests should be co-located with the code they test
Follow the existing pattern of*_test.gofiles for test naming
Use fixtures for realistic test data as defined in/utils/unittest/
Files:
engine/execution/state/bootstrap/bootstrap_test.gofvm/evm/evm_test.go
{module,engine,cmd}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
All major processing components must implement the
Componentinterface from/module/component/component.goto ensure consistent lifecycle management and graceful shutdown patterns
Files:
engine/execution/state/bootstrap/bootstrap_test.go
{crypto,fvm,ledger,access,engine}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation
Files:
engine/execution/state/bootstrap/bootstrap_test.gofvm/evm/evm_test.go
{network,engine,consensus}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Network messages must be authenticated and validated
Files:
engine/execution/state/bootstrap/bootstrap_test.go
{crypto,fvm,ledger,storage}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Security checks for cryptographic misuse must be enforced using gosec
Files:
fvm/evm/evm_test.go
{storage,ledger,execution,fvm}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
State consistency is paramount; use proper synchronization primitives
Files:
fvm/evm/evm_test.go
🧠 Learnings (1)
📚 Learning: 2025-12-23T00:28:41.005Z
Learnt from: CR
Repo: onflow/flow-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T00:28:41.005Z
Learning: Applies to {storage,ledger,execution,fvm}/**/*.go : State consistency is paramount; use proper synchronization primitives
Applied to files:
utils/unittest/execution_state.go
🧬 Code graph analysis (1)
utils/unittest/execution_state.go (1)
model/flow/chain.go (1)
Sandboxnet(26-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (37)
- GitHub Check: Lint (./)
- GitHub Check: Lint (./insecure/)
- GitHub Check: Lint (./integration/)
- GitHub Check: Unit Tests (module)
- GitHub Check: Unit Tests (network/p2p/node)
- GitHub Check: Unit Tests (engine/common)
- GitHub Check: Unit Tests (network/test/cohort1)
- GitHub Check: Unit Tests (network/p2p/connection)
- GitHub Check: Unit Tests (utils)
- GitHub Check: Unit Tests (network)
- GitHub Check: Unit Tests (cmd)
- GitHub Check: Unit Tests (network/p2p/scoring)
- GitHub Check: Unit Tests (engine)
- GitHub Check: Unit Tests (network/p2p)
- GitHub Check: Unit Tests (network/alsp)
- GitHub Check: Unit Tests (network/test/cohort2)
- GitHub Check: Unit Tests (engine/execution)
- GitHub Check: Unit Tests (engine/execution/ingestion)
- GitHub Check: Unit Tests (others)
- GitHub Check: Unit Tests (engine/verification)
- GitHub Check: Unit Tests (module/dkg)
- GitHub Check: Unit Tests (engine/execution/computation)
- GitHub Check: Unit Tests (engine/access)
- GitHub Check: Unit Tests (admin)
- GitHub Check: Unit Tests (engine/collection)
- GitHub Check: Unit Tests (consensus)
- GitHub Check: Unit Tests (storage)
- GitHub Check: Unit Tests (state)
- GitHub Check: Unit Tests (fvm)
- GitHub Check: Unit Tests (engine/consensus)
- GitHub Check: Unit Tests (ledger)
- GitHub Check: Integration Tests Others (integration)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/rpc_inspector)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/scoring)
- GitHub Check: Unit Tests Insecure (insecure)
- GitHub Check: Docker Build
- GitHub Check: analyze-code (go)
🔇 Additional comments (6)
engine/execution/state/bootstrap/bootstrap_test.go (1)
59-63: State commitment updates align with EVM contract changes.The updated expected state commitment hex values in
TestBootstrapLedger_ZeroTokenSupplyandTestBootstrapLedger_EmptyTransactionare consistent with the addition of theisPaused()function in the EVM contract, which modifies the genesis state. These changes correctly reflect the new contract state.Also applies to: 108-111
fvm/evm/stdlib/contract.cdc (2)
203-206: Pause guards correctly placed on COA state-modifying operations.The pause checks are appropriately added at the entry points of state-modifying operations (
deposit,withdraw,deploy,call) forCadenceOwnedAccountandEVMAddress.deposit. These guards will prevent state changes when the EVM is paused, aligning with the PR objectives.Also applies to: 510-513, 538-541, 573-576, 594-597
712-715: Pause guards onrunandbatchRunentry points.The pause checks on
EVM.runandEVM.batchRuncorrectly prevent external transaction execution when the EVM is paused. Note thatdryRunanddryCallare intentionally not guarded, which is appropriate since they are read-only operations that don't persist state changes.Also applies to: 774-777
fvm/evm/evm_test.go (2)
4330-4358: Test setup correctly pauses EVM operations.The test setup stores
trueat/storage/evmOperationsPausedto enable the pause state, which aligns with theisPaused()implementation in the contract. This approach simulates the governance-controlled pause mechanism.
4379-4427: Comprehensive test coverage for EVM pause functionality.The test suite thoroughly covers all pause-guarded entry points:
- EOA deposit
EVM.runEVM.batchRunCadenceOwnedAccount.deployCadenceOwnedAccount.callCadenceOwnedAccount.depositCadenceOwnedAccount.withdrawEach test correctly verifies that the operation fails with the expected error message "EVM operations are temporarily paused".
Also applies to: 4429-4491, 4493-4566, 4568-4619, 4621-4678, 4680-4724, 4726-4767
utils/unittest/execution_state.go (1)
85-96: Verify Sandboxnet genesis commitment consistency.The genesis state commitments for Mainnet, Testnet, and the default case have been updated. However, Sandboxnet's commitment at line 93 remains unchanged. Since Sandboxnet is an active, supported network in the codebase, please confirm whether this inconsistency is intentional—if Sandboxnet should receive the same update as Testnet, its genesis commitment may need updating.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
m-Peter
force-pushed
the
mpeter/evm-pause-functionality
branch
from
eaf419c to
4cfe1ad
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @fvm/evm/stdlib/contract.cdc:
- Around line 1059-1074: The isPaused() function dereferences
evmOperationsPaused without handling the case where borrow<&Bool>(from:
/storage/evmOperationsPaused) returns nil; update isPaused to safely handle a
nil optional and return false by default when the storage value is absent.
Locate the isPaused function and the evmOperationsPaused variable, check for nil
(or use optional unwrapping/guard) before dereferencing, and return false if
evmOperationsPaused is nil so no panic occurs.
🧹 Nitpick comments (1)
fvm/evm/evm_test.go (1)
4375-4738: Comprehensive test coverage for pause functionality.The subtests thoroughly cover all guarded entry points:
- EOA deposit
- EVM.run
- EVM.batchRun
- COA deploy, call, deposit, withdraw
All assertions correctly verify the expected error message "EVM operations are temporarily paused".
Consider adding tests for:
- Read-only operations during pause - Verify that
dryRun,dryCall, and balance queries still work when EVM is paused (confirming read-only mode).- Resume functionality - Test that operations work again after setting the flag back to
false.Also, minor variable shadowing at line 4511 where
txis reused inside the loop:tx := testAccount.PrepareSignAndEncodeTx(...)This shadows the outer
txvariable, which could be confusing. Consider renaming toinnerTxfor clarity.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
engine/execution/state/bootstrap/bootstrap_test.gofvm/evm/evm_test.gofvm/evm/stdlib/contract.cdcutils/unittest/execution_state.go
🚧 Files skipped from review as they are similar to previous changes (1)
- engine/execution/state/bootstrap/bootstrap_test.go
🧰 Additional context used
📓 Path-based instructions (5)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/coding_conventions.mdc)
Follow Go coding conventions as documented in @docs/agents/CodingConventions.md
Follow Go coding standards and conventions as documented in @docs/agents/GoDocs.md
**/*.go: Follow the existing module structure in/module/,/engine/,/model/and use dependency injection patterns for component composition
Implement proper interfaces before concrete types
Follow Go naming conventions and the project's coding style defined in /docs/CodingConventions.md
Use mock generators: runmake generate-mocksafter interface changes
All inputs must be considered potentially byzantine; error classification is context-dependent and no code path is safe unless explicitly proven and documented
Use comprehensive error wrapping for debugging; avoidfmt.Errorf, useirrecoverablepackage for exceptions
NEVER log and continue on best effort basis; ALWAYS explicitly handle errors
Uses golangci-lint with custom configurations (.golangci.yml) and custom linters for Flow-specific conventions (struct write checking)
Files:
utils/unittest/execution_state.gofvm/evm/evm_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Unit tests should be co-located with the code they test
Follow the existing pattern of*_test.gofiles for test naming
Use fixtures for realistic test data as defined in/utils/unittest/
Files:
fvm/evm/evm_test.go
{crypto,fvm,ledger,storage}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Security checks for cryptographic misuse must be enforced using gosec
Files:
fvm/evm/evm_test.go
{crypto,fvm,ledger,access,engine}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation
Files:
fvm/evm/evm_test.go
{storage,ledger,execution,fvm}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
State consistency is paramount; use proper synchronization primitives
Files:
fvm/evm/evm_test.go
🧠 Learnings (1)
📚 Learning: 2025-12-23T00:28:41.005Z
Learnt from: CR
Repo: onflow/flow-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T00:28:41.005Z
Learning: Applies to {storage,ledger,execution,fvm}/**/*.go : State consistency is paramount; use proper synchronization primitives
Applied to files:
utils/unittest/execution_state.go
🧬 Code graph analysis (1)
utils/unittest/execution_state.go (1)
model/flow/chain.go (1)
Sandboxnet(26-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (37)
- GitHub Check: Lint (./)
- GitHub Check: Lint (./integration/)
- GitHub Check: Lint (./insecure/)
- GitHub Check: Unit Tests (admin)
- GitHub Check: Unit Tests (engine)
- GitHub Check: Unit Tests (fvm)
- GitHub Check: Unit Tests (network/p2p/node)
- GitHub Check: Unit Tests (network/alsp)
- GitHub Check: Unit Tests (engine/execution/ingestion)
- GitHub Check: Unit Tests (network/test/cohort1)
- GitHub Check: Unit Tests (utils)
- GitHub Check: Unit Tests (cmd)
- GitHub Check: Unit Tests (network)
- GitHub Check: Unit Tests (engine/access)
- GitHub Check: Unit Tests (others)
- GitHub Check: Unit Tests (engine/common)
- GitHub Check: Unit Tests (engine/consensus)
- GitHub Check: Unit Tests (network/p2p)
- GitHub Check: Unit Tests (storage)
- GitHub Check: Unit Tests (engine/execution)
- GitHub Check: Unit Tests (network/p2p/connection)
- GitHub Check: Unit Tests (module)
- GitHub Check: Unit Tests (engine/collection)
- GitHub Check: Unit Tests (module/dkg)
- GitHub Check: Unit Tests (network/p2p/scoring)
- GitHub Check: Unit Tests (network/test/cohort2)
- GitHub Check: Unit Tests (engine/execution/computation)
- GitHub Check: Unit Tests (state)
- GitHub Check: Unit Tests (engine/verification)
- GitHub Check: Unit Tests (ledger)
- GitHub Check: Unit Tests (consensus)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/scoring)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/rpc_inspector)
- GitHub Check: Unit Tests Insecure (insecure)
- GitHub Check: Integration Tests Others (integration)
- GitHub Check: Docker Build
- GitHub Check: analyze-code (go)
🔇 Additional comments (9)
utils/unittest/execution_state.go (1)
26-26: State commitment updates look correct for the EVM pause feature.The updated hex values for
GenesisStateCommitmentHex, Testnet, and the default case are properly formatted 32-byte hex strings. These changes are expected when the bootstrap state changes due to the new EVM contract functionality.Note that the Sandboxnet value at line 93 was not updated. If Sandboxnet should also reflect the new EVM pause capability, this may need to be updated as well.
Also applies to: 90-90, 95-95
fvm/evm/stdlib/contract.cdc (7)
203-206: Pause guard correctly placed at the entry point.The check is appropriately positioned at the start of the function, before any state modifications occur. This ensures deposits are blocked when EVM is paused.
510-513: Pause guard is appropriate; note minor redundancy.This guard is correctly placed. Note that
self.address().deposit()at line 514 also checksisPaused(), creating a double-check. This is acceptable for defense-in-depth but could be optimized if desired.
538-541: Pause guard correctly placed for withdraw operation.The check appropriately blocks withdrawals when EVM is paused, maintaining read-only mode semantics.
573-576: Pause guard correctly placed for deploy operation.Contract deployments are properly blocked when EVM is paused.
594-597: Pause guard correctly placed for call operation.EVM calls that can modify state are properly blocked when paused.
712-715: Pause guard correctly placed for EVM.run.The main entry point for executing RLP-encoded EVM transactions is properly guarded.
774-777: Pause guard correctly placed for EVM.batchRun.Batch transaction execution is properly blocked when EVM is paused.
fvm/evm/evm_test.go (1)
4330-4374: Test setup correctly simulates governance-based EVM pause.The test properly:
- Creates a transaction to store
trueat/storage/evmOperationsPausedon the service account- Updates the snapshot for subsequent subtests to inherit the paused state
- Uses
t.Parallel()appropriately at the test function levelThis effectively simulates how the Governance Committee would pause EVM operations.
holyfuchs
left a comment
holyfuchs
left a comment
There was a problem hiding this comment.
NFT deposits and withdrawals seem to still be possible from CadenceOwnedAccount
flow-go/fvm/evm/stdlib/contract.cdc
Lines 632 to 638 in 4cfe1ad
as well as depositTokens and withdrawTokens with a feeProvider
flow-go/fvm/evm/stdlib/contract.cdc
Lines 665 to 671 in 4cfe1ad
| if EVM.isPaused() { | ||
| panic("EVM operations are temporarily paused") | ||
| } | ||
|
|
There was a problem hiding this comment.
could use pre here instead
pre {
!EVM.isPaused(): "EVM operations are temporarily paused"
}There was a problem hiding this comment.
Nice one 👌 Updated in 790e1f2 .
The NFT & token deposits and withdrawals are performed by the Bridge using a CadenceOwnedAccount, and since we have added the pause logic in the state-mutating methods of CadenceOwnedAccount, it seems redundant to do so in the mentioned methods. Nevertheless, it's better to be safe than sorry, so I added the pause condition check in all relevant methods.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fvm/evm/stdlib/contract.cdc (1)
708-716: Add pause guard tocreateCadenceOwnedAccountto be consistent with other state-modifying operations.This function calls
InternalEVM.createCadenceOwnedAccount(uuid: acc.uuid)which modifies EVM state by allocating a new address. All other state-modifying InternalEVM operations in this file (deposit, withdraw, deploy, call, run, batchRun) are guarded with!EVM.isPaused()checks, but this function is not. To align with the PR objective of making EVM state read-only during pause and maintain consistency across the codebase, add a pre-condition check:Suggested change
access(all) fun createCadenceOwnedAccount(): `@CadenceOwnedAccount` { pre { !EVM.isPaused(): "EVM operations are temporarily paused" } let acc <-create CadenceOwnedAccount() let addr = InternalEVM.createCadenceOwnedAccount(uuid: acc.uuid) acc.initAddress(addressBytes: addr) emit CadenceOwnedAccountCreated(address: acc.address().toString(), uuid: acc.uuid) return <-acc }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
engine/execution/state/bootstrap/bootstrap_test.gofvm/evm/stdlib/contract.cdcutils/unittest/execution_state.go
🚧 Files skipped from review as they are similar to previous changes (2)
- utils/unittest/execution_state.go
- engine/execution/state/bootstrap/bootstrap_test.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-13T17:39:03.954Z
Learnt from: m-Peter
Repo: onflow/flow-go PR: 8334
File: fvm/evm/stdlib/contract.cdc:1067-1074
Timestamp: 2026-01-13T17:39:03.954Z
Learning: In Cadence (e.g., .cdc files like contract.cdc), dereferencing a nil optional reference in a boolean comparison yields false instead of panicking. For example, *nilRef == true evaluates to false. Treat this as a language-specific nuance: while nil-dereferenced in a comparison won’t panic, do not rely on this to replace explicit nil checks in all contexts. When reviewing code, note that optional booleans involved in comparisons may be false when nil, and add explicit nil guards where correctness requires it beyond simple boolean contexts.
Applied to files:
fvm/evm/stdlib/contract.cdc
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (37)
- GitHub Check: Lint (./integration/)
- GitHub Check: Lint (./)
- GitHub Check: Lint (./insecure/)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/scoring)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/rpc_inspector)
- GitHub Check: Unit Tests Insecure (insecure)
- GitHub Check: Unit Tests (module)
- GitHub Check: Unit Tests (network/test/cohort1)
- GitHub Check: Unit Tests (network)
- GitHub Check: Unit Tests (others)
- GitHub Check: Unit Tests (engine/collection)
- GitHub Check: Unit Tests (network/p2p/node)
- GitHub Check: Unit Tests (network/test/cohort2)
- GitHub Check: Unit Tests (engine/consensus)
- GitHub Check: Unit Tests (network/p2p/connection)
- GitHub Check: Unit Tests (engine/common)
- GitHub Check: Unit Tests (network/p2p/scoring)
- GitHub Check: Unit Tests (network/p2p)
- GitHub Check: Unit Tests (engine/execution/ingestion)
- GitHub Check: Unit Tests (module/dkg)
- GitHub Check: Unit Tests (network/alsp)
- GitHub Check: Unit Tests (engine/verification)
- GitHub Check: Unit Tests (admin)
- GitHub Check: Unit Tests (storage)
- GitHub Check: Unit Tests (engine)
- GitHub Check: Unit Tests (ledger)
- GitHub Check: Unit Tests (fvm)
- GitHub Check: Unit Tests (engine/execution)
- GitHub Check: Unit Tests (engine/access)
- GitHub Check: Unit Tests (engine/execution/computation)
- GitHub Check: Unit Tests (utils)
- GitHub Check: Unit Tests (cmd)
- GitHub Check: Unit Tests (consensus)
- GitHub Check: Unit Tests (state)
- GitHub Check: Integration Tests Others (integration)
- GitHub Check: Docker Build
- GitHub Check: analyze-code (go)
🔇 Additional comments (9)
fvm/evm/stdlib/contract.cdc (9)
203-206: LGTM!The pre-condition guard correctly prevents deposits when EVM is paused, using the appropriate
EVM.isPaused()reference from within the struct context.
510-513: LGTM!Consistent pause guard implementation for COA deposits.
538-541: LGTM!Correctly guards withdrawals to maintain read-only mode when paused.
573-576: LGTM!Contract deployments correctly guarded as state-modifying operations.
594-597: LGTM!State-modifying calls correctly guarded while read-only
dryCall(line 611) remains accessible.
637-640: LGTM!All NFT and token bridging operations (
depositNFT,withdrawNFT,depositTokens,withdrawTokens) are correctly guarded to prevent state modifications when paused.Also applies to: 660-663, 678-681, 694-697
728-731: LGTM!Transaction execution correctly guarded. Note that
mustRunis transitively protected through its call toself.run().
790-793: LGTM!Batch transaction execution properly guarded with consistent pattern.
1075-1088: LGTM!Clean implementation using
copy<Bool>with nil-coalescing as recommended. Theviewmodifier is appropriate, and the documentation clearly explains the pause semantics.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
m-Peter
force-pushed
the
mpeter/evm-pause-functionality
branch
from
d45edc9 to
37eb6fa
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
engine/execution/state/bootstrap/bootstrap_test.go (2)
108-113: State commitment update aligns with EVM pause functionality.The updated hex value correctly reflects the post-transaction state commitment with the new EVM pause flag in the genesis state.
Same optional suggestion applies here: consider defining this commitment as a named constant in
utils/unittest/execution_state.goalongside other test-scenario commitments to follow the established pattern shown ingenesisCommitHexByChainID().,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/execution/state/bootstrap/bootstrap_test.go` around lines 108 - 113, The test TestBootstrapLedger_EmptyTransaction embeds a raw hex state commitment; extract this value into a named constant in utils/unittest/execution_state.go (alongside the other test commitments used by genesisCommitHexByChainID()), e.g. GenesisCommitWithEvmPause or EmptyTxnStateCommitment, and update the test to reference that constant before calling flow.ToStateCommitment to keep test data centralized and consistent with genesisCommitHexByChainID().
59-64: State commitment update is correct for the EVM pause feature.The updated hex value is a valid 32-byte state commitment reflecting the new genesis state with EVM pause functionality.
Consider extracting to a named constant for maintainability. Looking at the pattern in
utils/unittest/execution_state.go(e.g.,GenesisStateCommitmentHex), these test-specific commitments could be similarly defined as named constants. This would make future updates easier to track and maintain.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/execution/state/bootstrap/bootstrap_test.go` around lines 59 - 64, The test hardcodes the 32-byte state commitment hex in TestBootstrapLedger_ZeroTokenSupply; extract that hex string into a named constant (e.g., ZeroTokenSupplyStateCommitmentHex) in the same unittest constants area (follow the pattern used by GenesisStateCommitmentHex in utils/unittest/execution_state.go), then update TestBootstrapLedger_ZeroTokenSupply to decode and use that constant (replace the inline string passed to hex.DecodeString and keep the existing flow.ToStateCommitment call and error handling).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@engine/execution/state/bootstrap/bootstrap_test.go`:
- Around line 108-113: The test TestBootstrapLedger_EmptyTransaction embeds a
raw hex state commitment; extract this value into a named constant in
utils/unittest/execution_state.go (alongside the other test commitments used by
genesisCommitHexByChainID()), e.g. GenesisCommitWithEvmPause or
EmptyTxnStateCommitment, and update the test to reference that constant before
calling flow.ToStateCommitment to keep test data centralized and consistent with
genesisCommitHexByChainID().
- Around line 59-64: The test hardcodes the 32-byte state commitment hex in
TestBootstrapLedger_ZeroTokenSupply; extract that hex string into a named
constant (e.g., ZeroTokenSupplyStateCommitmentHex) in the same unittest
constants area (follow the pattern used by GenesisStateCommitmentHex in
utils/unittest/execution_state.go), then update
TestBootstrapLedger_ZeroTokenSupply to decode and use that constant (replace the
inline string passed to hex.DecodeString and keep the existing
flow.ToStateCommitment call and error handling).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fe353210-2009-4853-bbec-e8bfa0a3f1df
📒 Files selected for processing (1)
engine/execution/state/bootstrap/bootstrap_test.go
7 participants
Closes: #8311
Closes: #8317
Reject all EVM transactions, when EVM has been paused. EVM state enters a read-only mode, and cannot be updated by the public.
Summary by CodeRabbit
New Features
Tests
Chores