Update to Cadence v1.7.0 - #877
Conversation
WalkthroughUpdate Go toolchain to 1.25.0, perform a broad dependency refresh (Flow/Cadence, flow-go/sdk/emulator, go-ethereum, OpenTelemetry, google/gRPC/protobuf, golang.org/x/*, and various utilities), adjust CI/linter configs, minor refactors, and add Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant RemoteCadenceArch
Note over RemoteCadenceArch: New exported method Name() added\nreturns constant from evm/precompiles
Caller->>RemoteCadenceArch: Name()
RemoteCadenceArch-->>Caller: evmPrecompiles.CADENCE_ARCH_PRECOMPILE_NAME
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (1)
✨ Finishing touches
🧪 Generate unit tests
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
go.mod (1)
1-228: Update CI and Docker to Go 1.25 toolchain
- go.mod declares go 1.25.0 — Dockerfile (line 3) currently:
FROM golang:1.23 as app-builder→ change togolang:1.25.- Bump any CI workflow references (actions/setup-go go-version, runner/base images, other Dockerfiles) to 1.25 so pipeline images match go.mod.
🧹 Nitpick comments (3)
go.mod (1)
148-149: Align Flow core-contracts/templates with tests to avoid drift.Root uses older commits than tests. Use the same revisions everywhere to prevent subtle integration mismatches.
Apply (to match tests/go.mod):
-github.com/onflow/flow-core-contracts/lib/go/contracts v1.7.4-0.20250825173510-91e6f28b0224 // indirect -github.com/onflow/flow-core-contracts/lib/go/templates v1.7.2-0.20250825173510-91e6f28b0224 // indirect +github.com/onflow/flow-core-contracts/lib/go/contracts v1.7.4-0.20250905214304-819c6e946442 // indirect +github.com/onflow/flow-core-contracts/lib/go/templates v1.7.2-0.20250905214304-819c6e946442 // indirecttests/go.mod (2)
222-227: Align OTel exporters with core v1.37.0.Same consistency suggestion as root.
Apply:
-go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.31.0 // indirect -go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.31.0 // indirect +go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.37.0 // indirect +go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.37.0 // indirect
159-160: Align flow-core-contracts versions across modulestests/go.mod pins newer pseudo-versions than root go.mod — keep them identical to avoid dependency surprises.
- go.mod (lines 148–149):
github.com/onflow/flow-core-contracts/lib/go/contracts v1.7.4-0.20250825173510-91e6f28b0224 // indirect
github.com/onflow/flow-core-contracts/lib/go/templates v1.7.2-0.20250825173510-91e6f28b0224 // indirect- tests/go.mod (lines 159–160):
github.com/onflow/flow-core-contracts/lib/go/contracts v1.7.4-0.20250905214304-819c6e946442 // indirect
github.com/onflow/flow-core-contracts/lib/go/templates v1.7.2-0.20250905214304-819c6e946442 // indirectAction: align the versions (update tests/go.mod to match root or update root to the newer pseudo-version) and run go mod tidy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
go.mod(8 hunks)tests/go.mod(8 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#738
File: .github/workflows/ci.yml:19-19
Timestamp: 2025-01-29T17:20:28.143Z
Learning: Go 1.23 was released and is available for use in CI/CD pipelines and Docker images.
📚 Learning: 2025-01-29T17:20:28.143Z
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#738
File: .github/workflows/ci.yml:19-19
Timestamp: 2025-01-29T17:20:28.143Z
Learning: Go 1.23 was released and is available for use in CI/CD pipelines and Docker images.
Applied to files:
go.mod
📚 Learning: 2025-03-07T01:35:09.751Z
Learnt from: peterargue
PR: onflow/flow-evm-gateway#772
File: services/requester/keystore/key_store.go:50-62
Timestamp: 2025-03-07T01:35:09.751Z
Learning: In the flow-evm-gateway codebase, panics are acceptable in scenarios where immediate detection of critical bugs is desired during development and testing, particularly for invariant violations that should never occur in a correctly functioning system (e.g., when a key is available but locked in the keystore implementation).
Applied to files:
go.modtests/go.mod
🔇 Additional comments (7)
go.mod (5)
14-15: Non-release Flow-Go pseudo-version — confirm intent.You’re pinning to a dev-pebble commit. If that’s required for Cadence 1.7.0, fine; otherwise prefer a release to reduce churn.
7-7: Deps bump LGTM.go-ethereum v1.16.3, testify v1.11.1, grpc v1.75.0 look fine.
Also applies to: 23-23, 27-28
6-6: Don't drop Pebble v1 — it's imported directly.go.mod requires github.com/cockroachdb/pebble v1.1.5 while github.com/cockroachdb/pebble/v2 v2.0.6 is only indirect. Multiple files import the v1 path (e.g. storage/index.go, storage/pebble/db.go, storage/pebble/transactions.go, services/ingestion/engine.go, storage/mocks/*, storage/register_delta_test.go) — removing v1 will break builds. If you want a single major in the binary, migrate these imports to pebble/v2 or update the dependency that pulls in v2.
Likely an incorrect or invalid review comment.
201-206: Incorrect recommendation — OTLP exporters are v0.x, not v1.37.0otlptrace and otlptracegrpc latest release is v0.60.0 (as of 2025-09-10); do not change them to v1.37.0. Either update the exporter lines in go.mod to v0.60.0 or upgrade go.opentelemetry.io/otel to the matching release (v1.38.0) that bundles v0.60.0. File: go.mod (lines 201-206).
Likely an incorrect or invalid review comment.
3-3: Fix invalidgodirective (must be major.minor).
go 1.25.0will fail module parsing. Usego 1.25and, if you want to pin toolchain patch, add a separatetoolchaindirective.Apply:
-go 1.25.0 +go 1.25 +toolchain go1.25.0⛔ Skipped due to learnings
Learnt from: m-Peter PR: onflow/flow-evm-gateway#738 File: .github/workflows/ci.yml:19-19 Timestamp: 2025-01-29T17:20:28.143Z Learning: Go 1.23 was released and is available for use in CI/CD pipelines and Docker images.Learnt from: m-Peter PR: onflow/flow-evm-gateway#738 File: .github/workflows/ci.yml:19-19 Timestamp: 2025-01-29T17:20:28.143Z Learning: Go 1.23 is scheduled to be released on August 13, 2024, and should not be used in production before its official release.tests/go.mod (2)
6-7: Deps bump LGTM for tests.go-ethereum v1.16.3, cadence v1.7.0, emulator v1.7.0, flow-go-sdk v1.8.1, testify v1.11.1, x/sync v0.16.0.
Also applies to: 10-13, 15-16
248-249: grpc/protobuf updates LGTM.Minor bumps, no action needed.
| go 1.23.7 | ||
|
|
||
| toolchain go1.23.8 | ||
| go 1.25.0 |
There was a problem hiding this comment.
Fix invalid go directive (must be major.minor).
Same as root: change to go 1.25 and optionally add toolchain.
Apply:
-go 1.25.0
+go 1.25
+toolchain go1.25.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.
| go 1.25.0 | |
| go 1.25 | |
| toolchain go1.25.0 |
🤖 Prompt for AI Agents
In tests/go.mod around line 3, the go directive is invalid ("go 1.25.0") and
must be major.minor; change it to "go 1.25" (and optionally add a // toolchain
directive if you want to pin the toolchain), updating the file so the third line
reads the corrected go directive.
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 (2)
services/requester/remote_cadence_arch.go (2)
60-67: Returning 0 gas on error undercharges the precompile.If
runCallerrors,RequiredGasreturns 0, allowing execution with no gas charge. Fall back to a safe upper bound instead (e.g.,cadenceArchCallGasLimit).func (rca *RemoteCadenceArch) RequiredGas(input []byte) uint64 { evmResult, err := rca.runCall(input) if err != nil { - return 0 + // Fallback to an upper bound to avoid undercharging on transient failures. + return cadenceArchCallGasLimit } return evmResult.GasConsumed }
108-116: Add a timeout to the remote script execution.External call uses
context.Background()(no deadline). Add a bounded timeout to prevent hangs.- scriptResult, err := rca.client.ExecuteScriptAtBlockHeight( - context.Background(), + ctx, cancel := context.WithTimeout(context.Background(), cadenceArchScriptTimeout) + defer cancel() + scriptResult, err := rca.client.ExecuteScriptAtBlockHeight( + ctx, rca.blockHeight, replaceAddresses(dryRunScript, rca.chainID), []cadence.Value{hexEncodedTx, hexEncodedAddress}, )Additions outside this hunk:
// at top imports import ( // ... "time" ) // near other consts const cadenceArchScriptTimeout = 10 * time.Second
🧹 Nitpick comments (3)
.golangci.toml (1)
3-6: Formatter enablement is valid; consider setting local prefixes.
formatters.enable = ["goimports"]is valid in v2. Optionally setformatters.settings.goimports.local-prefixesto keep repo imports grouped.
Docs for v2 formatters. (golangci-lint.run)Apply if desired:
[formatters] enable = [ "goimports", ] +[formatters.settings.goimports] +local-prefixes = [ + "github.com/onflow/flow-evm-gateway", +].github/workflows/ci.yml (1)
20-23: Optional: derive Go version from go.mod to avoid drift.Using
go-version-file: go.modremoves the need to maintainGO_VERSIONand picks the latest patch automatically.- - uses: actions/setup-go@v5 - with: - go-version: ${{ env.GO_VERSION }} + - uses: actions/setup-go@v5 + with: + go-version-file: go.modDocs: setup-go supports go-version-file and enables caching by default. (github.com)
services/requester/remote_cadence_arch.go (1)
72-75: Map cache may be accessed concurrently.If
RemoteCadenceArchinstances are shared, unsynchronizedcachedCallsrisks data races. Consider guarding withsync.RWMutex(orsync.Map).Minimal change:
type RemoteCadenceArch struct { blockHeight uint64 client *CrossSporkClient chainID flow.ChainID cachedCalls map[string]evmTypes.Data + mu sync.RWMutex } // in Run - if result, ok := rca.cachedCalls[key]; ok { + rca.mu.RLock() + result, ok := rca.cachedCalls[key] + rca.mu.RUnlock() + if ok { return result, nil } // in runCall (before write) - rca.cachedCalls[key] = evmResult.ReturnedData + rca.mu.Lock() + rca.cachedCalls[key] = evmResult.ReturnedData + rca.mu.Unlock()And add
syncto imports.Also applies to: 123-125
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/ci.yml(2 hunks).golangci.toml(1 hunks)services/requester/remote_cadence_arch.go(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#738
File: .github/workflows/ci.yml:19-19
Timestamp: 2025-01-29T17:20:28.143Z
Learning: Go 1.23 was released and is available for use in CI/CD pipelines and Docker images.
📚 Learning: 2025-01-29T17:20:28.143Z
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#738
File: .github/workflows/ci.yml:19-19
Timestamp: 2025-01-29T17:20:28.143Z
Learning: Go 1.23 was released and is available for use in CI/CD pipelines and Docker images.
Applied to files:
.github/workflows/ci.yml
🔇 Additional comments (3)
.golangci.toml (1)
1-1: Config v2 switch looks correct.
version = '2'matches the new GolangCI-Lint v2 config schema.
See: official config docs. (golangci-lint.run).github/workflows/ci.yml (1)
35-39: Action and linter versions are consistent.Using
golangci/golangci-lint-action@v8withversion: v2.4aligns with the action’s v8 compatibility matrix (supports golangci-lint v2.x) and accepted version formats.
Refs: action README (compat/inputs). (github.com)services/requester/remote_cadence_arch.go (1)
52-54: New Name() satisfies the updated interface.Method addition is straightforward and stable.
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 (2)
api/pull.go (2)
386-393: Fix data race: iterating overapi.filterswithout holding the mutex can panic
filterExpiryCheckerranges overapi.filterswhile other goroutines add/remove filters underapi.mux. This can cause "concurrent map iteration and map write" panics. Collect expired IDs under lock, then uninstall outside the lock.Apply this diff:
func (api *PullAPI) filterExpiryChecker() { - for range time.Tick(api.config.FilterExpiry) { - for id, f := range api.filters { - if f.expired() { - api.logger.Debug().Str("id", string(id)).Msg("filter expired") - api.UninstallFilter(id) - } - } - } + ticker := time.NewTicker(api.config.FilterExpiry) + defer ticker.Stop() + for range ticker.C { + var expired []rpc.ID + api.mux.Lock() + for id, f := range api.filters { + if f.expired() { + expired = append(expired, id) + } + } + api.mux.Unlock() + for _, id := range expired { + api.logger.Debug().Str("id", string(id)).Msg("filter expired") + api.UninstallFilter(id) + } + } }
400-406: Off-by-one on max filters check; consider returning an error
if len(api.filters) > maxFiltersallows one extra filter (10,001st). Use>=. Also, returning""without error makes upstream return success with an empty ID.Apply this minimal fix now:
- if len(api.filters) > maxFilters { - return "" - } + if len(api.filters) >= maxFilters { + api.logger.Warn().Int("limit", maxFilters).Msg("max filters limit reached") + return "" + }Optionally, we should propagate an error to callers so JSON-RPC returns a proper error instead of an empty ID. I can draft that change if desired.
🧹 Nitpick comments (3)
cmd/run/cmd.go (2)
218-225: Make flag parsing resilient to casing/whitespaceNormalize the input to avoid surprising rejections like "Tx-Seal" or trailing spaces.
- switch txStateValidation { + txStateValidation = strings.ToLower(strings.TrimSpace(txStateValidation)) + switch txStateValidation {
127-129: Wrong variable in error messageThis path validates
cloudKMSKeybut printskey(COA private key), misleading users.- return fmt.Errorf("wrong format for Cloud KMS key: %s", key) + return fmt.Errorf("wrong format for Cloud KMS key: %s", cloudKMSKey)models/transaction.go (1)
153-155: Confirm encode/decode round-trip for DirectCallSwitching to
dc.Encode()should keepMarshalBinaryconsistent withDirectCallFromEncoded. Recommend a quick round-trip test to guard against future Cadence upgrades.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/pull.go(1 hunks)cmd/run/cmd.go(1 hunks)models/transaction.go(3 hunks)tests/tx_batching_test.go(8 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#738
File: .github/workflows/ci.yml:19-19
Timestamp: 2025-01-29T17:20:28.143Z
Learning: Go 1.23 was released and is available for use in CI/CD pipelines and Docker images.
🧬 Code graph analysis (1)
cmd/run/cmd.go (1)
config/config.go (3)
LocalIndexValidation(29-29)TxStateValidation(26-26)TxSealValidation(30-30)
⏰ 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). (2)
- GitHub Check: Lint
- GitHub Check: Test
🔇 Additional comments (12)
api/pull.go (1)
181-181: Nil-safe boolean simplification LGTMThe new
full := fullTx != nil && *fullTxis correct and clearer.cmd/run/cmd.go (1)
218-225: Switch-based mapping reads well and preserves behaviorMapping user flag to
cfg.TxStateValidationvia switch is clear; default error remains intact.models/transaction.go (2)
82-85: Access path update LGTM; verify To() semantics with deploy callsUsing
dc.EmptyToField()aligns with promoted API. Please ensure we still returnnilfor deployments and the correcttootherwise (add/keep a focused test).
105-107: Gas accessor update LGTMReturning
dc.GasLimitmatches the promoted field.tests/tx_batching_test.go (8)
21-23: Import reorder OKNo behavioral change.
172-176: Block height API change handled correctlyUsing
startBlock.Height/endBlock.Heightaligns with the struct update.
255-256: Height lookup update LGTM
latestBlock.Height + 1is correct with the new API.
271-271: Second height lookup update LGTMConsistent with the first change.
369-370: Height lookup update LGTM (recent interval test)Matches the new
Block.Heightfield.
385-385: Second height lookup update LGTM (recent interval test)Consistent change.
484-485: Height lookup update LGTM (non-recent interval test)Consistent with other tests.
500-500: Second height lookup update LGTM (non-recent interval test)Consistent change.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
bootstrap/bootstrap.go (1)
432-432: Consider logging Close() errors at debug level instead of fully discarding.Swallowing Close() is fine in shutdown paths, but a debug log helps post-mortems if Close() indicates a partial/forced teardown.
- _ = b.profiler.Close() + if cerr := b.profiler.Close(); cerr != nil { + b.logger.Debug().Err(cerr).Msg("forced profiler close returned error") + }cmd/blocks/cmd.go (1)
24-26: Defer Close() with error ignored is acceptable here; optionally surface at debug.This CLI is read-only, but Pebble Close() errors can be interesting under FS issues. Consider a low-noise debug log.
- defer func() { - _ = pebbleDB.Close() - }() + defer func() { + if err := pebbleDB.Close(); err != nil { + log.Debug().Err(err).Msg("pebble close error") + } + }()api/server.go (2)
251-271: Close the original request body, not the re-wrapped NopCloser.After reassigning r.Body, the Close() currently targets the new NopCloser (a no-op), not the original body. Close the original to be precise and avoid subtle leaks under proxies/middleboxes.
- if b, err := io.ReadAll(r.Body); err == nil { + origBody := r.Body + if b, err := io.ReadAll(origBody); err == nil { _ = json.Unmarshal(b, &requestBody) ... - r.Body = io.NopCloser(bytes.NewBuffer(b)) - _ = r.Body.Close() + r.Body = io.NopCloser(bytes.NewBuffer(b)) + _ = origBody.Close() }
327-330: Ignoring Close() errors during forced shutdown is reasonable; consider errors.Is on Shutdown.LGTM to discard Close() errors here. As a tiny robustness nit, use errors.Is for the Shutdown timeout check.
- if err != nil && err == ctx.Err() { + if err != nil && (errors.Is(err, context.DeadlineExceeded) || errors.Is(err, ctx.Err())) { h.logger.Warn().Msg("HTTP server graceful shutdown timed out") _ = h.server.Close() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
api/server.go(2 hunks)bootstrap/bootstrap.go(1 hunks)cmd/blocks/cmd.go(2 hunks)
⏰ 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). (1)
- GitHub Check: Test
🔇 Additional comments (1)
cmd/blocks/cmd.go (1)
10-12: Import reordering looks fine.
2 participants
Description
Automatically update to:
Summary by CodeRabbit
Chores
Tests
Minor