[FVM] Resolve reusable runtime environment import cycle#8303
[FVM] Resolve reusable runtime environment import cycle#8303janezpodhostnik merged 4 commits intomasterfrom
Conversation
📝 WalkthroughWalkthroughAdds interface-based abstractions for the Cadence runtime and its pool (ReusableCadenceRuntime, ReusableCadenceRuntimePool), moves default environment params to package Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as FVM caller (script/tx)
participant RuntimePool as ReusableCadenceRuntimePool
participant Reusable as ReusableCadenceRuntime
participant Env as environment.Environment
participant EVM as EVM setup/exec
Caller->>RuntimePool: Borrow(fvmEnv: Env)
RuntimePool-->>Caller: ReusableCadenceRuntime
Caller->>Reusable: CadenceScriptEnv() / CadenceTXEnv()
Caller->>EVM: SetupEnvironment(using cadence env)
Caller->>Reusable: ExecuteScript / NewTransactionExecutor / InvokeContractFunction / ReadStored
EVM-->>Caller: execution results
Caller->>RuntimePool: Return(Reusable)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (4)**/*.go📄 CodeRabbit inference engine (.cursor/rules/coding_conventions.mdc)
Files:
{crypto,fvm,ledger,storage}/**/*.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
{crypto,fvm,ledger,access,engine}/**/*.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
{storage,ledger,execution,fvm}/**/*.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧬 Code graph analysis (1)fvm/environment/env.go (3)
⏰ 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). (36)
🔇 Additional comments (4)
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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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/environment/env.go:
- Around line 126-130: Fix the duplicate "a" typo in the doc comments for
CadenceTXEnv and CadenceScriptEnv: change "returns a a cadence
runtime.Environment" to "returns a cadence runtime.Environment" in the comments
above the CadenceTXEnv and CadenceScriptEnv method declarations.
🧹 Nitpick comments (1)
fvm/runtime/reusable_cadence_runtime_pool.go (1)
118-126: Verify intent of zero pool size in DefaultRuntimeParams.
DefaultRuntimeParamscreates a pool withpoolSize: 0, which means no runtime instances are cached—eachBorrowcreates a new runtime, andReturndiscards it. If this is intentional for default/testing scenarios, consider adding a brief comment to document the rationale.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
fvm/context.gofvm/environment/env.gofvm/environment/mock/environment.gofvm/environment/mock/reusable_cadence_runtime.gofvm/environment/mock/reusable_cadence_runtime_interface.gofvm/environment/mock/reusable_cadence_runtime_pool.gofvm/environment/programs_test.gofvm/environment/runtime.gofvm/fvm_test.gofvm/runtime/reusable_cadence_runtime.gofvm/runtime/reusable_cadence_runtime_pool.gofvm/script.gofvm/transactionInvoker.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:
fvm/script.gofvm/fvm_test.gofvm/environment/mock/reusable_cadence_runtime_pool.gofvm/environment/mock/environment.gofvm/environment/env.gofvm/runtime/reusable_cadence_runtime_pool.gofvm/environment/programs_test.gofvm/environment/mock/reusable_cadence_runtime_interface.gofvm/context.gofvm/environment/runtime.gofvm/environment/mock/reusable_cadence_runtime.gofvm/transactionInvoker.gofvm/runtime/reusable_cadence_runtime.go
{crypto,fvm,ledger,storage}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Security checks for cryptographic misuse must be enforced using gosec
Files:
fvm/script.gofvm/fvm_test.gofvm/environment/mock/reusable_cadence_runtime_pool.gofvm/environment/mock/environment.gofvm/environment/env.gofvm/runtime/reusable_cadence_runtime_pool.gofvm/environment/programs_test.gofvm/environment/mock/reusable_cadence_runtime_interface.gofvm/context.gofvm/environment/runtime.gofvm/environment/mock/reusable_cadence_runtime.gofvm/transactionInvoker.gofvm/runtime/reusable_cadence_runtime.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/script.gofvm/fvm_test.gofvm/environment/mock/reusable_cadence_runtime_pool.gofvm/environment/mock/environment.gofvm/environment/env.gofvm/runtime/reusable_cadence_runtime_pool.gofvm/environment/programs_test.gofvm/environment/mock/reusable_cadence_runtime_interface.gofvm/context.gofvm/environment/runtime.gofvm/environment/mock/reusable_cadence_runtime.gofvm/transactionInvoker.gofvm/runtime/reusable_cadence_runtime.go
{storage,ledger,execution,fvm}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
State consistency is paramount; use proper synchronization primitives
Files:
fvm/script.gofvm/fvm_test.gofvm/environment/mock/reusable_cadence_runtime_pool.gofvm/environment/mock/environment.gofvm/environment/env.gofvm/runtime/reusable_cadence_runtime_pool.gofvm/environment/programs_test.gofvm/environment/mock/reusable_cadence_runtime_interface.gofvm/context.gofvm/environment/runtime.gofvm/environment/mock/reusable_cadence_runtime.gofvm/transactionInvoker.gofvm/runtime/reusable_cadence_runtime.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/fvm_test.gofvm/environment/programs_test.go
🧠 Learnings (2)
📚 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 **/*.go : Use mock generators: run `make generate-mocks` after interface changes
Applied to files:
fvm/environment/mock/reusable_cadence_runtime_pool.gofvm/environment/mock/environment.gofvm/environment/mock/reusable_cadence_runtime_interface.gofvm/environment/mock/reusable_cadence_runtime.go
📚 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:
fvm/transactionInvoker.go
🧬 Code graph analysis (10)
fvm/fvm_test.go (1)
fvm/context.go (1)
DefaultEnvironmentParams(90-103)
fvm/environment/mock/reusable_cadence_runtime_pool.go (5)
fvm/environment/runtime.go (1)
ReusableCadenceRuntimePool(5-12)fvm/runtime/reusable_cadence_runtime_pool.go (2)
ReusableCadenceRuntimePool(12-29)NewReusableCadenceRuntimePool(52-63)fvm/environment/env.go (2)
Environment(14-86)ReusableCadenceRuntime(90-134)fvm/environment/mock/environment.go (1)
Environment(41-43)fvm/environment/mock/reusable_cadence_runtime.go (1)
ReusableCadenceRuntime(19-21)
fvm/environment/mock/environment.go (3)
fvm/environment/env.go (2)
Environment(14-86)ReusableCadenceRuntime(90-134)fvm/environment/mock/reusable_cadence_runtime.go (1)
ReusableCadenceRuntime(19-21)fvm/runtime/reusable_cadence_runtime.go (1)
ReusableCadenceRuntime(11-17)
fvm/environment/env.go (3)
fvm/environment/mock/reusable_cadence_runtime.go (1)
ReusableCadenceRuntime(19-21)fvm/runtime/reusable_cadence_runtime.go (1)
ReusableCadenceRuntime(11-17)fvm/environment/mock/environment.go (1)
Environment(41-43)
fvm/runtime/reusable_cadence_runtime_pool.go (3)
fvm/environment/runtime.go (2)
Runtime(19-23)RuntimeParams(14-16)fvm/environment/env.go (2)
ReusableCadenceRuntime(90-134)Environment(14-86)fvm/runtime/reusable_cadence_runtime.go (1)
ReusableCadenceRuntime(11-17)
fvm/environment/programs_test.go (1)
fvm/context.go (1)
DefaultEnvironmentParams(90-103)
fvm/environment/mock/reusable_cadence_runtime_interface.go (2)
fvm/environment/env.go (1)
Environment(14-86)fvm/environment/mock/environment.go (1)
Environment(41-43)
fvm/environment/runtime.go (5)
fvm/environment/env.go (2)
Environment(14-86)ReusableCadenceRuntime(90-134)fvm/environment/mock/environment.go (1)
Environment(41-43)fvm/environment/mock/reusable_cadence_runtime.go (1)
ReusableCadenceRuntime(19-21)fvm/runtime/reusable_cadence_runtime.go (1)
ReusableCadenceRuntime(11-17)fvm/runtime/reusable_cadence_runtime_pool.go (1)
ReusableCadenceRuntimePool(12-29)
fvm/transactionInvoker.go (3)
fvm/environment/env.go (1)
ReusableCadenceRuntime(90-134)fvm/environment/mock/reusable_cadence_runtime.go (1)
ReusableCadenceRuntime(19-21)fvm/runtime/reusable_cadence_runtime.go (1)
ReusableCadenceRuntime(11-17)
fvm/runtime/reusable_cadence_runtime.go (2)
fvm/environment/runtime.go (1)
Runtime(19-23)fvm/environment/env.go (2)
Environment(14-86)ReusableCadenceRuntime(90-134)
🔇 Additional comments (19)
fvm/context.go (1)
88-103: LGTM! Well-structured default environment parameters.The new
DefaultEnvironmentParams()function provides a clean way to construct base environment settings. Usingflow.Mainnetas the default chain is reasonable since the actual chain can be overridden viaWithChain()context option when needed.fvm/script.go (1)
205-209: LGTM! Correctly uses the new interface method.The change from field access (
rt.ScriptRuntimeEnv) to method call (rt.CadenceScriptEnv()) correctly aligns with the newReusableCadenceRuntimeinterface abstraction.fvm/environment/programs_test.go (1)
143-146: LGTM! Test updated for new function location.The change correctly uses
fvm.DefaultEnvironmentParams()instead of the previousenvironment.DefaultEnvironmentParams(), consistent with the refactoring that moved this function to the fvm package.fvm/fvm_test.go (1)
3873-3878: LGTM! Tests updated consistently.Both test cases correctly use
fvm.DefaultEnvironmentParams()instead of the previousenvironment.DefaultEnvironmentParams(). The changes are consistent with the refactoring and maintain test functionality.Also applies to: 3902-3907
fvm/transactionInvoker.go (2)
72-72: LGTM! Improved abstraction with interface usage.The change from concrete type
*reusableRuntime.ReusableCadenceRuntimeto interface typeenvironment.ReusableCadenceRuntimefollows good software engineering practices by programming to interfaces rather than implementations. This provides better flexibility and testability.
187-198: LGTM! Consistent migration to interface methods.The changes from field access (
.TxRuntimeEnv) to method calls (.CadenceTXEnv()) correctly align with the newReusableCadenceRuntimeinterface. Both occurrences inpreprocessTransactionBody()andExecuteTransactionBody()are updated consistently.Also applies to: 254-265
fvm/runtime/reusable_cadence_runtime.go (2)
8-8: Import and interface wiring looks correct.The import of
environmentpackage and the compile-time interface assertionvar _ environment.ReusableCadenceRuntime = (*ReusableCadenceRuntime)(nil)properly ensures that the concrete type satisfies the interface defined in the environment package, breaking the import cycle.Also applies to: 16-16, 19-19
49-55: LGTM!The accessor methods correctly expose the internal runtime environments, matching the interface contract defined in
environment.ReusableCadenceRuntime.fvm/environment/mock/environment.go (2)
252-269: Auto-generated mock correctly reflects interface changes.The
BorrowCadenceRuntimemock now returnsenvironment.ReusableCadenceRuntimeinterface type, matching the updatedEnvironmentinterface. Based on learnings, ensuremake generate-mockswas run after interface changes.
1493-1496: Mock signature updated correctly.
ReturnCadenceRuntimenow acceptsenvironment.ReusableCadenceRuntime, consistent with the interface definition.fvm/environment/mock/reusable_cadence_runtime_pool.go (1)
1-52: Auto-generated mock for new pool interface.The mock correctly implements the
ReusableCadenceRuntimePoolinterface with proper testify/mock patterns. Based on learnings, this was generated viamake generate-mocks.fvm/runtime/reusable_cadence_runtime_pool.go (2)
31-32: Interface assertion ensures type safety.The compile-time assertion
var _ environment.ReusableCadenceRuntimePool = (*ReusableCadenceRuntimePool)(nil)ensures the concrete pool type satisfies the interface contract.
86-104: Borrow method correctly handles interface return type.The method returns
environment.ReusableCadenceRuntimeinterface while internally creating concrete*ReusableCadenceRuntimeinstances. TheSetFvmEnvironmentcall on line 102 correctly wires the environment before returning.fvm/environment/runtime.go (2)
3-12: Clean interface abstraction for runtime pooling.The
ReusableCadenceRuntimePoolinterface cleanly abstracts the pool operations, allowing different pool implementations (including mocks) to be injected.
35-37: The nil environment handling inBorrowCadenceRuntimeis by design.SetEnvironmentis called before the environment is returned fromNewScriptEnv/NewTransactionEnv(facade_env.go line 155), and the nil environment passed during pool return operations is intentionally supported by theReturnmethod (reusable_cadence_runtime_pool.go line 109). No code path callsBorrowCadenceRuntimebeforeSetEnvironmentis called.fvm/environment/env.go (2)
24-25: Interface-based runtime methods.The
Environmentinterface now uses theReusableCadenceRuntimeinterface type for borrow/return operations, enabling proper abstraction and breaking the import cycle.
88-134: Well-designed interface for reusable runtime abstraction.The
ReusableCadenceRuntimeinterface properly encapsulates the Cadence runtime operations needed by the FVM, including script/transaction execution, contract invocation, and environment access. This cleanly resolves the import cycle mentioned in the PR objective.fvm/environment/mock/reusable_cadence_runtime.go (1)
1-190: Auto-generated mock correctly implements ReusableCadenceRuntime interface.All interface methods are properly mocked with standard testify/mock patterns, including proper handling of function-based return values and direct value casting. Based on learnings, this was generated via
make generate-mocksafter interface changes.fvm/environment/mock/reusable_cadence_runtime_interface.go (1)
1-190: LGTM! Auto-generated mock follows standard patterns.The mock implementation is correctly generated by mockery and follows all standard testify/mock patterns. Each method properly:
- Delegates to the mock recorder via
Called(...)- Guards against missing return values with panic messages
- Handles both function and direct value returns with type assertions
- The constructor correctly registers test hooks and cleanup
Based on learnings, remember to run
make generate-mocksif theReusableCadenceRuntimeInterfaceinterface changes in the future.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
janezpodhostnik
requested review from
fxamacker,
holyfuchs,
turbolent and
zhangchiqing
fxamacker
left a comment
fxamacker
left a comment
There was a problem hiding this comment.
Nice! I left a minor comment/question.
5 participants
ref: #8301
Resolve the issue where the runtime had to create a copy of the Environment interface.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.