Skip to content

Conversation

@m-Peter
Copy link
Collaborator

@m-Peter m-Peter commented Jan 13, 2026

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

    • Governance-controlled EVM pause: public pause view and guards enforcing read-only behavior with clear "EVM operations are temporarily paused" errors.
  • Tests

    • Expanded pause validation across deposits, runs, batch operations, deployments, calls, withdrawals, and bridge actions.
    • Duplicate test instance added (additional coverage).
  • Chores

    • Updated genesis/state commitment values and adjusted related test expectations.

✏️ Tip: You can customize this high-level summary in your review settings.

@m-Peter m-Peter self-assigned this Jan 13, 2026
@m-Peter m-Peter requested a review from a team as a code owner January 13, 2026 17:08
@github-actions
Copy link
Contributor

github-actions bot commented Jan 13, 2026

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 13, 2026

📝 Walkthrough

Walkthrough

Adds a governance-controlled EVM pause flag (isPaused()) to the EVM system contract and enforces early errors when paused across EVM entry points; adds tests asserting paused behavior (with a duplicated test insertion); updates hardcoded genesis/bootstrap state-commitment bytes and bootstrap test expectations.

Changes

Cohort / File(s) Summary
EVM contract
fvm/evm/stdlib/contract.cdc
Add view fun isPaused(): Bool and insert early pause checks that panic/error when paused on deposit, withdraw, deploy, call, run, batchRun, and bridge-related methods (NFT/token bridge accessors).
EVM tests
fvm/evm/evm_test.go
Add TestEVMPauseFunctionality suite covering paused-state errors across EOA deposit, EVM.run, EVM.batchRun, and CadenceOwnedAccount deploy/call/deposit/withdraw. Note: the test suite is inserted twice (duplicate).
Bootstrap / Genesis state
engine/execution/state/bootstrap/bootstrap_test.go, utils/unittest/execution_state.go
Update expected state-commitment bytes in bootstrap tests and update GenesisStateCommitmentHex and chain-specific genesisCommitHexByChainID values. No API signatures changed.

Sequence Diagram(s)

sequenceDiagram
    participant User as User Tx
    participant EVM as EVM Contract
    participant Storage as /storage (evmOperationsPaused)
    participant Internal as InternalEVM

    User->>EVM: submit transaction (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
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • peterargue
  • zhangchiqing
  • turbolent

Poem

🐇 I saw a tiny switch so bright,
I tucked the EVM in for night.
When chaos comes I hold the line,
Then lift the latch when skies align. 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 3
❌ Failed checks (2 warnings, 1 inconclusive)
Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The fvm/evm/evm_test.go file contains a duplicated TestEVMPauseFunctionality function, and utils/unittest/execution_state.go updates appear unrelated to the pause functionality objectives. Remove the duplicate test function and clarify the purpose of the genesis state commitment updates; ensure all changes directly support the pause functionality requirements.
Docstring Coverage ⚠️ Warning Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Linked Issues check ❓ Inconclusive The code changes add pause guards to EVM operations and update test fixtures, but it is unclear if the service account authorization requirement from #8311 is implemented. Verify that the pause functionality is properly restricted to the service account with appropriate authorization checks in the contract code.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: adding functionality to pause EVM transactions, which is the primary objective of the pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

🧹 Recent nitpick comments
fvm/evm/evm_test.go (2)

4506-4523: Consider renaming loop variable to avoid shadowing outer tx.

The loop variable tx (line 4511) shadows the outer tx variable that's later reassigned at line 4543. While this works correctly due to scoping, using a different name like evmTx or txPayload would improve readability.

♻️ Suggested naming improvement
 for i := 0; i < batchCount; i++ {
     num := int64(i)
     // prepare batch of transaction payloads
-    tx := testAccount.PrepareSignAndEncodeTx(t,
+    evmTx := testAccount.PrepareSignAndEncodeTx(t,
         testContract.DeployedAt.ToCommon(),
         testContract.MakeCallData(t, "storeWithLog", big.NewInt(num)),
         big.NewInt(0),
         uint64(100_000),
         big.NewInt(1),
     )

     // build txs argument
     txBytes[i] = cadence.NewArray(
-        unittest.BytesToCdcUInt8(tx),
+        unittest.BytesToCdcUInt8(evmTx),
     ).WithType(stdlib.EVMTransactionBytesCadenceType)
 }

4700-4740: Good test coverage - Consider adding a resume test for completeness.

The test suite thoroughly covers the pause behavior across all EVM entry points. For even more robust coverage, consider adding a final subtest that removes the pause flag and verifies operations resume successfully. This would confirm the pause mechanism is bidirectional.

Example structure for a resume test:

t.Run("testing operations resume after unpausing EVM", func(t *testing.T) {
    // Remove the pause flag
    unpauseCode := []byte(fmt.Sprintf(`
        import EVM from %s
        transaction(){
            prepare(account: auth(Storage) &Account) {
                account.storage.load<Bool>(from: /storage/evmOperationsPaused)
            }
        }
    `, sc.EVMContract.Address.HexWithPrefix()))
    // Execute unpause and verify a simple EVM operation succeeds
})

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a2f833f and 19bedfd.

📒 Files selected for processing (2)
  • engine/execution/state/bootstrap/bootstrap_test.go
  • fvm/evm/evm_test.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: run make generate-mocks after 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; avoid fmt.Errorf, use irrecoverable package 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:

  • fvm/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.go files 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
🧬 Code graph analysis (1)
fvm/evm/evm_test.go (6)
fvm/evm/emulator/emulator.go (1)
  • Emulator (26-29)
model/flow/chain.go (2)
  • Chain (263-275)
  • ChainID (14-14)
fvm/systemcontracts/system_contracts.go (1)
  • SystemContractsForChain (300-307)
fvm/storage/snapshot/snapshot_tree.go (1)
  • SnapshotTree (15-19)
fvm/evm/testutils/contract.go (2)
  • TestContract (19-23)
  • MakeCallData (25-31)
fvm/evm/types/balance.go (1)
  • BalanceToBigInt (55-57)
⏰ 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 (network/alsp)
  • GitHub Check: Unit Tests (network/test/cohort1)
  • GitHub Check: Unit Tests (network/p2p/connection)
  • GitHub Check: Unit Tests (network/test/cohort2)
  • GitHub Check: Unit Tests (network/p2p)
  • GitHub Check: Unit Tests (network)
  • GitHub Check: Unit Tests (module)
  • GitHub Check: Unit Tests (others)
  • GitHub Check: Unit Tests (network/p2p/node)
  • GitHub Check: Unit Tests (module/dkg)
  • GitHub Check: Unit Tests (network/p2p/scoring)
  • GitHub Check: Unit Tests (engine)
  • GitHub Check: Unit Tests (engine/execution/computation)
  • GitHub Check: Unit Tests (ledger)
  • GitHub Check: Unit Tests (engine/verification)
  • GitHub Check: Unit Tests (engine/execution/ingestion)
  • GitHub Check: Unit Tests (engine/collection)
  • GitHub Check: Unit Tests (cmd)
  • GitHub Check: Unit Tests (engine/execution)
  • GitHub Check: Unit Tests (storage)
  • GitHub Check: Unit Tests (engine/consensus)
  • GitHub Check: Unit Tests (engine/access)
  • GitHub Check: Unit Tests (fvm)
  • GitHub Check: Unit Tests (engine/common)
  • GitHub Check: Unit Tests (admin)
  • GitHub Check: Unit Tests (utils)
  • GitHub Check: Unit Tests (state)
  • GitHub Check: Unit Tests (consensus)
  • GitHub Check: Integration Tests Others (integration)
  • 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: Docker Build
  • GitHub Check: analyze-code (go)
🔇 Additional comments (2)
fvm/evm/evm_test.go (2)

4330-4374: LGTM - Test setup and structure are well-designed.

The test setup correctly pauses EVM operations by storing a flag in the service account's storage, then executes multiple subtests against the paused state. The use of RunWithNewEnvironment provides proper isolation, and the snapshot is correctly updated after the pause transaction.


4330-4331: AI summary inconsistency: No duplicate test function found.

The AI-generated summary claims there is a "second identical TestEVMPauseFunctionality(t *testing.T) block" but only one instance exists in the provided code (lines 4330-4740). The test implementation appears correct and complete.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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 variable storedValues in batchRun test.

The storedValues slice 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/evmOperationsPaused to false or 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

📥 Commits

Reviewing files that changed from the base of the PR and between 6d5c1e9 and eaf419c.

📒 Files selected for processing (4)
  • engine/execution/state/bootstrap/bootstrap_test.go
  • fvm/evm/evm_test.go
  • fvm/evm/stdlib/contract.cdc
  • utils/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: run make generate-mocks after 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; avoid fmt.Errorf, use irrecoverable package 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.go
  • fvm/evm/evm_test.go
  • utils/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.go files for test naming
Use fixtures for realistic test data as defined in /utils/unittest/

Files:

  • engine/execution/state/bootstrap/bootstrap_test.go
  • fvm/evm/evm_test.go
{module,engine,cmd}/**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

All major processing components must implement the Component interface from /module/component/component.go to 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.go
  • fvm/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_ZeroTokenSupply and TestBootstrapLedger_EmptyTransaction are consistent with the addition of the isPaused() 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) for CadenceOwnedAccount and EVMAddress.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 on run and batchRun entry points.

The pause checks on EVM.run and EVM.batchRun correctly prevent external transaction execution when the EVM is paused. Note that dryRun and dryCall are 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 true at /storage/evmOperationsPaused to enable the pause state, which aligns with the isPaused() 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.run
  • EVM.batchRun
  • CadenceOwnedAccount.deploy
  • CadenceOwnedAccount.call
  • CadenceOwnedAccount.deposit
  • CadenceOwnedAccount.withdraw

Each 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-commenter
Copy link

codecov-commenter commented Jan 13, 2026

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
utils/unittest/execution_state.go 0.00% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

@m-Peter m-Peter force-pushed the mpeter/evm-pause-functionality branch from eaf419c to 4cfe1ad Compare January 13, 2026 17:20
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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:

  1. Read-only operations during pause - Verify that dryRun, dryCall, and balance queries still work when EVM is paused (confirming read-only mode).
  2. Resume functionality - Test that operations work again after setting the flag back to false.

Also, minor variable shadowing at line 4511 where tx is reused inside the loop:

tx := testAccount.PrepareSignAndEncodeTx(...)

This shadows the outer tx variable, which could be confusing. Consider renaming to innerTx for clarity.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between eaf419c and 4cfe1ad.

📒 Files selected for processing (4)
  • engine/execution/state/bootstrap/bootstrap_test.go
  • fvm/evm/evm_test.go
  • fvm/evm/stdlib/contract.cdc
  • utils/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: run make generate-mocks after 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; avoid fmt.Errorf, use irrecoverable package 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.go
  • fvm/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.go files 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 checks isPaused(), 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:

  1. Creates a transaction to store true at /storage/evmOperationsPaused on the service account
  2. Updates the snapshot for subsequent subtests to inherit the paused state
  3. Uses t.Parallel() appropriately at the test function level

This effectively simulates how the Governance Committee would pause EVM operations.

Copy link

@holyfuchs holyfuchs left a comment

Choose a reason for hiding this comment

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

NFT deposits and withdrawals seem to still be possible from CadenceOwnedAccount

access(all)
fun depositNFT(
nft: @{NonFungibleToken.NFT},
feeProvider: auth(FungibleToken.Withdraw) &{FungibleToken.Provider}
) {
EVM.borrowBridgeAccessor().depositNFT(nft: <-nft, to: self.address(), feeProvider: feeProvider)
}

as well as depositTokens and withdrawTokens with a feeProvider
access(all)
fun depositTokens(
vault: @{FungibleToken.Vault},
feeProvider: auth(FungibleToken.Withdraw) &{FungibleToken.Provider}
) {
EVM.borrowBridgeAccessor().depositTokens(vault: <-vault, to: self.address(), feeProvider: feeProvider)
}

if EVM.isPaused() {
panic("EVM operations are temporarily paused")
}

Choose a reason for hiding this comment

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

could use pre here instead

pre {
    !EVM.isPaused(): "EVM operations are temporarily paused"
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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 to createCadenceOwnedAccount to 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

📥 Commits

Reviewing files that changed from the base of the PR and between 7e6e0c3 and a2f833f.

📒 Files selected for processing (3)
  • engine/execution/state/bootstrap/bootstrap_test.go
  • fvm/evm/stdlib/contract.cdc
  • utils/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 mustRun is transitively protected through its call to self.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. The view modifier 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 m-Peter requested a review from holyfuchs January 16, 2026 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Incident action plan] Add ability to disable EVM via service account multi-sig [Flow EVM] Add pause functionality on EVM system contract

6 participants