Use flow-go Components for composing #682
Conversation
WalkthroughThis pull request introduces a comprehensive refactoring of the EVM Gateway's architecture, focusing on component-based design, improved error handling, and streamlined lifecycle management. The changes span multiple files, including bootstrap, services, models, and tests. Key modifications include replacing the Changes
Assessment against linked issues
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
janezpodhostnik
force-pushed
the
janez/config-instance
branch
from
1d626c7 to
a2386c1
Compare
janezpodhostnik
changed the base branch from
janez/switch-to-flow-go-metrics-server
to
janez/config-instance-2
janezpodhostnik
changed the base branch from
janez/config-instance-2
to
feature/local-tx-reexecution
janezpodhostnik
force-pushed
the
janez/use-flow-go-components
branch
from
33d6278 to
67136f9
Compare
janezpodhostnik
force-pushed
the
janez/use-flow-go-components
branch
4 times, most recently
from
9e3947b to
50d77d2
Compare
janezpodhostnik
requested review from
peterargue,
m-Peter and
zhangchiqing
as code owners
m-Peter
force-pushed
the
feature/local-tx-reexecution
branch
from
2121836 to
a746c82
Compare
janezpodhostnik
force-pushed
the
janez/use-flow-go-components
branch
from
87a1102 to
453f201
Compare
There was a problem hiding this comment.
thanks for making this consistent with flow-go. will make it much easier to reason with.
a general comment about the component usage. In some places you used a component manager, and in others you wrote your own. I'm OK with custom handling where it's easier, but I think we should avoid non-idempotent Ready/Done methods
janezpodhostnik
force-pushed
the
janez/use-flow-go-components
branch
from
050ab5f to
93ee75c
Compare
janezpodhostnik
force-pushed
the
janez/use-flow-go-components
branch
from
93ee75c to
e9e537f
Compare
bootstrap/bootstrap.go
Outdated
| l.Info().Msg("bootstrap starting event ingestion") | ||
|
|
||
| // get latest cadence block from the network and the database | ||
| gatewayLatestBlock, err := fnb.Client.GetLatestBlock(context.Background(), true) |
There was a problem hiding this comment.
I believe this should be chainLatestBlock.
bootstrap/bootstrap.go
Outdated
|
|
||
| func (b *Bootstrap) StartMetricsServer(ctx context.Context) error { | ||
| b.logger.Info().Msg("bootstrap starting metrics server") | ||
| chainLatestHeight, err := fnb.Storages.Blocks.LatestCadenceHeight() |
There was a problem hiding this comment.
And this should be gatewayLatestHeight.
| return | ||
| } | ||
| for _, key := range account.Keys { | ||
| accountKeys = append(accountKeys, &AccountKey{ |
There was a problem hiding this comment.
We are missing this piece of code: https://github.com/onflow/flow-evm-gateway/blob/main/bootstrap/bootstrap.go#L217-L221, from this loop here. This is quite important for the setup that we have now on testnet & mainnet.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
api/server.go (1)
Line range hint
192-219: Avoid signaling readiness when the server fails to startThe
startupCompletedchannel is closed regardless of whether the server starts successfully or not. This could causeReady()to signal that the server is ready even if it failed to start due to an error or misconfiguration.Apply this diff to close
startupCompletedonly after the server has started successfully:func (h *Server) Start(ctx irrecoverable.SignalerContext) { - defer close(h.startupCompleted) if h.endpoint == "" || h.listener != nil { return // already running or not configured } + defer func() { + if err == nil { + close(h.startupCompleted) + } + }() // Initialize the server.
🧹 Nitpick comments (7)
api/profiler.go (1)
80-100: Refine Shutdown Logic inshutdownOnContextDoneThe
shutdownOnContextDonemethod may log errors even when there is no error (i.e.,err == nil). Adjust the error handling to log messages appropriately based on the presence of an error.Consider modifying the error handling as follows:
err := s.server.Shutdown(ctx) if err == nil { s.log.Info().Msg("Profiler server graceful shutdown completed") + return } - if errors.Is(err, ctx.Err()) { + if errors.Is(err, context.DeadlineExceeded) { s.log.Warn().Msg("Profiler server graceful shutdown timed out") err := s.server.Close() if err != nil { s.log.Err(err).Msg("error closing profiler server") } } else { s.log.Err(err).Msg("error shutting down profiler server") }services/requester/key_store_component.go (1)
78-102: Simplify Ready and Done Methods Using Sync MechanismsThe
ReadyandDonemethods use additional goroutines and channels to signal readiness and completion. Consider using synchronization primitives likesync.Onceor leveraging existing component patterns to simplify this logic.Refactor the methods to reduce complexity and potential overhead from unnecessary goroutines.
api/server.go (1)
245-253: Simplify theReadymethodConsider simplifying the
Readymethod by returning thestartupCompletedchannel directly, as it's already a channel that signals readiness.Apply this diff to simplify the method:
func (h *Server) Ready() <-chan struct{} { - ready := make(chan struct{}) - - go func() { - <-h.startupCompleted - close(ready) - }() - - return ready + return h.startupCompleted }bootstrap/bootstrap.go (3)
54-56: Consider renamingEVMGatewayNodeImptoEVMGatewayNodeImplfor consistencyIt's standard practice in Go to use
Implas a suffix for implementation structs. RenamingEVMGatewayNodeImptoEVMGatewayNodeImplwould improve clarity and adhere to conventions.
369-371: Wrap the error with context for better error handlingCurrently, the error returned from
fnb.Storages.Blocks.LatestCadenceHeight()is returned directly. Consider wrapping it with additional context to aid in debugging.Apply this diff:
if err != nil { - return nil, err + return nil, fmt.Errorf("failed to get latest cadence height: %w", err) }
402-403: Wrap the error with context for better error handlingInstead of returning the error directly from
replayer.NewCallTracerCollector, wrap it with context to improve debuggability.Apply this diff:
if err != nil { - return nil, err + return nil, fmt.Errorf("failed to create CallTracerCollector: %w", err) }tests/helpers.go (1)
187-199: Well-structured gateway initialization function.The function follows a clear sequence of steps and properly manages the node lifecycle. Consider adding a function comment to document its purpose and behavior.
Add a function comment:
+// startGateway initializes and starts the EVM gateway node. +// It blocks until the node is ready to accept requests. func startGateway(t *testing.T, ctx context.Context, cfg config.Config) {
📜 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 (18)
Makefile(0 hunks)api/profiler.go(1 hunks)api/server.go(8 hunks)api/stream.go(2 hunks)bootstrap/bootstrap.go(3 hunks)bootstrap/utils.go(0 hunks)cmd/run/cmd.go(1 hunks)go.mod(6 hunks)models/engine.go(0 hunks)models/mocks/Engine.go(0 hunks)models/stream.go(2 hunks)models/stream_test.go(5 hunks)services/ingestion/engine.go(5 hunks)services/ingestion/engine_test.go(17 hunks)services/requester/key_store_component.go(1 hunks)tests/go.mod(13 hunks)tests/helpers.go(1 hunks)tests/integration_test.go(4 hunks)
💤 Files with no reviewable changes (4)
- bootstrap/utils.go
- Makefile
- models/mocks/Engine.go
- models/engine.go
🔇 Additional comments (20)
models/stream.go (2)
25-41: Properly Build and Assign the Component ManagerIn the
NewPublisherfunction, theComponentManageris built and assigned to bothp.cmandp.Component. Confirm that this dual assignment is necessary and doesn't lead to redundant references or potential confusion in managing the component's lifecycle.Please confirm if assigning both
p.cmandp.Componentis required or if one reference suffices.
67-98:⚠️ Potential issueEnsure Proper Error Propagation in
publishWorkerWithin
publishWorker, when an error occurs durings.Notify(data),ctx.Throw(err)is called. Sincectx.Throw(err)signals an irrecoverable error, it's important to ensure that the goroutine exits immediately after throwing the error to prevent further execution.Consider adding a
returnstatement afterctx.Throw(err)to ensure the worker exits:if err != nil { p.log.Error().Err(err).Msg("failed to notify subscriber") ctx.Throw(err) + return true }Likely invalid or redundant comment.
api/profiler.go (1)
51-77:⚠️ Potential issueHandle Errors Appropriately in the
serveMethodIn the
servemethod, after callingctx.Throw(err), the function should return immediately to prevent further execution. Continuing after throwing an error might lead to unintended behavior.Apply this diff to ensure the function exits after throwing an error:
if err != nil { s.log.Err(err).Msg("failed to start the metrics server") ctx.Throw(err) return }Likely invalid or redundant comment.
services/requester/key_store_component.go (2)
104-131: Handle Missing Key Configuration increateSignerIn the
createSignerfunction, if neitherCOAKeynorCOACloudKMSKeyis provided, an error is returned. Ensure that this scenario is properly handled and that the error message provides clear guidance to the user.Confirm that the configuration validation occurs earlier in the application flow to prevent reaching this point without a valid key configuration.
37-59:⚠️ Potential issueEnsure Immediate Return After Throwing Errors in
StartIn the
Startmethod, after callingctx.Throw(err), the function should return immediately to prevent executing subsequent code that may depend on successful operations.Apply this diff to add missing return statements:
if err != nil { ctx.Throw(fmt.Errorf( "failed to get signer info account for address: %s, with: %w", k.config.COAAddress, err, )) + return } signer, err := createSigner(ctx, k.config, k.log) if err != nil { ctx.Throw(err) + return }Likely invalid or redundant comment.
cmd/run/cmd.go (1)
42-42: Include 'node' in the error message for clarityConsider modifying the error message to "failed to initialize node" for better clarity and consistency.
Apply this diff to make the change:
- builder.Logger.Fatal().Err(err).Msg("failed to initialize") + builder.Logger.Fatal().Err(err).Msg("failed to initialize node")services/ingestion/engine.go (1)
116-140: Refactoredrunmethod integration looks goodThe integration with the component framework and the updated error handling using
ctx.Throware appropriate. The use ofready()after initializing the subscriber ensures that the component correctly signals readiness.bootstrap/bootstrap.go (1)
486-487:⚠️ Potential issueFix typo and use variable in error message
There's a misspelling in the variable name
evmBlokcHeight; it should beevmBlockHeight. Also, use the variable in the error message instead of hardcoding0.Apply this diff:
- evmBlokcHeight := uint64(0) + evmBlockHeight := uint64(0) // ... - snapshot, err := registerStore.GetSnapshotAt(evmBlokcHeight) + snapshot, err := registerStore.GetSnapshotAt(evmBlockHeight) if err != nil { - return fmt.Errorf("could not get register snapshot at block height %d: %w", 0, err) + return fmt.Errorf("could not get register snapshot at block height %d: %w", evmBlockHeight, err) }Likely invalid or redundant comment.
models/stream_test.go (1)
105-112: Ensure correct capture of loop variable in closureWhen using closures inside loops, it's important to capture the loop variable correctly to avoid unexpected behavior. In this case,
jis correctly captured since there is no goroutine, so the current implementation is acceptable.tests/integration_test.go (4)
85-85: LGTM! The gateway startup has been simplified.The change improves code readability by removing channel-based synchronization in favor of a more straightforward
startGatewayfunction call.
187-187: Consistent with previous changes.
276-276: Consistent with previous changes.
370-370: Consistent with previous changes.tests/helpers.go (1)
179-179: Consistent with previous changes.services/ingestion/engine_test.go (1)
73-77: LGTM! Improved publisher initialization with proper context handling.The change ensures that publishers are properly initialized and started with the irrecoverable context, aligning with the PR's goal of improving error handling.
go.mod (1)
11-11: Verify the flow-go dependency version.The flow-go dependency has been updated to a very specific version. Please ensure this version is stable and compatible with the current changes.
Run this script to check the dependency's stability:
✅ Verification successful
The flow-go dependency version is valid and stable.
The version points to a specific verified commit (72adf9e522c4) from January 2, 2025, which adds implementation for transaction statuses data providers. The changes are well-structured, properly tested, and merged through a reviewed pull request.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the flow-go dependency version and its stability. # Check if the version exists in the repository gh api repos/onflow/flow-go/git/refs/tags/v0.38.0-preview.0.4.0.20250102180624-72adf9e522c4 # Check for any known issues with this version gh api search/issues -f q='repo:onflow/flow-go is:issue label:bug v0.38.0-preview.0.4.0.20250102180624-72adf9e522c4'Length of output: 566
Script:
#!/bin/bash # Check if the commit hash exists and list recent tags gh api repos/onflow/flow-go/commits/72adf9e522c4 || echo "Commit not found" gh api repos/onflow/flow-go/tags?per_page=5 # Check go.sum for this dependency grep "github.com/onflow/flow-go@" go.sum || echo "No matching entry in go.sum"Length of output: 85691
tests/go.mod (4)
226-226: LGTM! Good addition of Ginkgo testing framework.Adding Ginkgo v2 is a good choice for behavior-driven development (BDD) style testing, which aligns well with the component-based architecture being implemented.
295-296: LGTM! Essential dependencies for component framework.The addition of
go.uber.org/digandgo.uber.org/fxis aligned with the PR objective of integrating the flow-go component model for improved resource management.
31-46: Verify the necessity of AWS SDK dependencies.Multiple AWS SDK v2 packages have been added as indirect dependencies. Since these are marked as indirect, verify if they are actually required and through which direct dependency they were introduced.
✅ Verification successful
AWS SDK dependencies are correctly marked as indirect
These AWS SDK packages are transitive dependencies pulled in through the Flow ecosystem dependencies (flow-go, flow-emulator, etc.). There is no direct usage of AWS services in the codebase, confirming these are correctly marked as indirect dependencies and require no further verification.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find which direct dependencies are pulling in AWS SDK # Search for AWS SDK imports in the codebase rg --type go "github.com/aws/aws-sdk-go-v2"Length of output: 43
Script:
#!/bin/bash # Check root go.mod and list direct dependencies echo "=== Root go.mod ===" if [ -f "go.mod" ]; then cat go.mod fi echo -e "\n=== Looking for AWS/S3 related code ===" # Search for S3, AWS, or cloud storage related terms rg -i "s3|aws|cloud.*storage" --type go echo -e "\n=== Direct dependencies in tests/go.mod ===" cat tests/go.mod | awk '/^[[:space:]]*[^[:space:]\/].*[[:space:]]v[0-9]/ && !/indirect/'Length of output: 17671
9-9: Verify compatibility with the updated Flow dependencies.The updates to flow-emulator and flow-go are using pre-release versions. Please ensure these versions are stable and compatible with your changes.
Also applies to: 11-11
✅ Verification successful
Development versions appear to be coordinated builds
The specified versions of flow-go and flow-emulator are very recent development builds (Jan 2-3, 2025) that appear to be coordinated, with flow-emulator being built against the new flow-go version. This aligns with the ongoing component model refactoring in flow-go. The dependency resolution is successful as evidenced by the presence in both go.mod and go.sum files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any known issues or breaking changes in the flow-go and flow-emulator versions # Check for any tagged releases or issues mentioning these specific versions gh issue list --repo onflow/flow-go --search "v0.38.0-preview.0.4.0.20250102180624-72adf9e522c4" gh issue list --repo onflow/flow-emulator --search "v1.1.1-0.20250103150158-e029ea3471fe"Length of output: 186
Script:
#!/bin/bash # Check recent issues and go.mod files for compatibility information # Check recent issues in both repositories echo "=== Recent Flow Go Issues ===" gh issue list --repo onflow/flow-go --limit 5 --state all echo -e "\n=== Recent Flow Emulator Issues ===" gh issue list --repo onflow/flow-emulator --limit 5 --state all # Search for go.mod files referencing these dependencies echo -e "\n=== Dependencies Usage Patterns ===" rg "github.com/onflow/flow-go.*v0.38.0-preview" -A 2 -B 2 rg "github.com/onflow/flow-emulator.*v1.1.1-0" -A 2 -B 2Length of output: 4601
| log zerolog.Logger | ||
|
|
||
| mux sync.RWMutex | ||
| subscribers map[Subscriber[T]]struct{} | ||
|
|
||
| publishChan chan T | ||
|
|
||
| publisherExited chan struct{} |
There was a problem hiding this comment.
Initialize All Fields in the Publisher Struct
The Publisher struct now includes additional fields such as Component, cm, log, publishChan, and publisherExited. Ensure that all these fields are properly initialized to avoid potential nil pointer dereferences.
Consider verifying the initialization in the constructor and ensuring that each field is appropriately set before use.
| Notify(data T) error | ||
| } | ||
|
|
||
| type Subscription[T any] struct { | ||
| logger zerolog.Logger | ||
| err chan error | ||
| callback func(data T) error | ||
| } | ||
|
|
||
| func NewSubscription[T any](logger zerolog.Logger, callback func(T) error) *Subscription[T] { | ||
| func NewSubscription[T any](callback func(T) error) *Subscription[T] { | ||
| return &Subscription[T]{ | ||
| logger: logger, | ||
| callback: callback, | ||
| err: make(chan error, 1), | ||
| } | ||
| } | ||
|
|
||
| func (b *Subscription[T]) Notify(data T) { | ||
| err := b.callback(data) | ||
| if err != nil { | ||
| select { | ||
| case b.err <- err: | ||
| default: | ||
| b.logger.Debug().Err(err).Msg("failed to send error to subscription") | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func (b *Subscription[T]) Error() <-chan error { | ||
| return b.err | ||
| func (b *Subscription[T]) Notify(data T) error { | ||
| return b.callback(data) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update All Implementations to Handle the Modified Notify Signature
The Notify method now returns an error. Ensure that all implementations of the Subscriber interface and related code handle this new signature correctly. This includes checking returned errors and updating any documentation or comments related to this method.
Please review all Subscriber implementations to accommodate the new error return type from Notify.
| select { | ||
| case <-p.publisherExited: | ||
| return | ||
| default: | ||
| p.publishChan <- data |
There was a problem hiding this comment.
Prevent Potential Blocking in the Publish Method
The Publish method writes to publishChan without checking if the channel is full. If publishChan is unbuffered or has a limited buffer, this could lead to blocking behavior. Consider adding a buffer to publishChan or handling cases where the channel is full to prevent potential deadlocks.
Apply this diff to add a buffer to publishChan:
- publishChan: make(chan T),
+ publishChan: make(chan T, bufferSize), // Define an appropriate buffer sizeAlternatively, handle the blocking scenario in the Publish method:
func (p *Publisher[T]) Publish(data T) {
select {
case <-p.publisherExited:
return
+ case p.publishChan <- data:
+ // Successfully published data
+ default:
+ // Handle channel being full
+ p.log.Warn().Msg("Publish channel is full; data may be dropped")
}
}Committable suggestion skipped: line range outside the PR's diff.
bootstrap/bootstrap.go
Outdated
| Uint64("chain-cadence-height", chainLatestBlock.Height). | ||
| Uint64("gateway-cadence-height", gatewayLatestHeight). | ||
| Uint64("missed-heights", chainLatestBlock.Height-gatewayLatestHeight). |
There was a problem hiding this comment.
Ensure safe subtraction to avoid underflow in "missed-heights" calculation
When calculating missed-heights, subtracting gatewayLatestHeight from chainLatestBlock.Height may result in underflow if gatewayLatestHeight is greater. Ensure that this calculation handles such cases safely.
Apply this diff:
l.Info().
Uint64("chain-cadence-height", chainLatestBlock.Height).
Uint64("gateway-cadence-height", gatewayLatestHeight).
- Uint64("missed-heights", chainLatestBlock.Height-gatewayLatestHeight).
+ Uint64("missed-heights", calculateMissedHeights(chainLatestBlock.Height, gatewayLatestHeight)).
Msg("indexing cadence height information")Add the helper function:
func calculateMissedHeights(chainHeight, gatewayHeight uint64) uint64 {
if chainHeight > gatewayHeight {
return chainHeight - gatewayHeight
}
return 0
}| Stringer("fvm_address_for_evm_storage_account", storageAddress). | ||
| Msgf("database initialized with cadence height: %d", cadenceHeight) | ||
|
|
There was a problem hiding this comment.
Use Msg instead of Msgf and include cadenceHeight as a field
zerolog does not support the Msgf method. Use Msg and include cadenceHeight as a field for structured logging.
Apply this diff:
logger.Info().
Stringer("fvm_address_for_evm_storage_account", storageAddress).
- Msgf("database initialized with cadence height: %d", cadenceHeight)
+ Uint64("cadence_height", cadenceHeight).
+ Msg("database initialized")📝 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.
| Stringer("fvm_address_for_evm_storage_account", storageAddress). | |
| Msgf("database initialized with cadence height: %d", cadenceHeight) | |
| Stringer("fvm_address_for_evm_storage_account", storageAddress). | |
| Uint64("cadence_height", cadenceHeight). | |
| Msg("database initialized") |
| defer func(batch *pebble.Batch) { | ||
| innerErr = batch.Close() | ||
| }(batch) |
There was a problem hiding this comment.
Fix improper use of defer to correctly close the batch
The deferred function is executed immediately due to the function call syntax (batch). To properly defer the execution until the surrounding function returns, remove the parameter from the anonymous function and the call.
Apply this diff:
batch := store.NewBatch()
- defer func(batch *pebble.Batch) {
+ defer func() {
innerErr = batch.Close()
- }(batch)
+ }()📝 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.
| defer func(batch *pebble.Batch) { | |
| innerErr = batch.Close() | |
| }(batch) | |
| defer func() { | |
| innerErr = batch.Close() | |
| }() |
| err := <-rpcSub.Err() | ||
| l.Debug().Err(err).Msg("client unsubscribed") |
There was a problem hiding this comment.
Handle errors from the subscription to prevent unhandled errors
Currently, the code only listens for errors from rpcSub.Err() and ignores potential errors from subs.Err(). This may lead to unhandled errors and resource leaks. Consider adding error handling for subs.Err().
Apply this diff:
go func() {
defer publisher.Unsubscribe(subs)
- err := <-rpcSub.Err()
- l.Debug().Err(err).Msg("client unsubscribed")
+ select {
+ case err := <-rpcSub.Err():
+ l.Debug().Err(err).Msg("client unsubscribed")
+ case err := <-subs.Err():
+ l.Error().Err(err).Msg("subscription error")
+ }
}()📝 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.
| err := <-rpcSub.Err() | |
| l.Debug().Err(err).Msg("client unsubscribed") | |
| go func() { | |
| defer publisher.Unsubscribe(subs) | |
| select { | |
| case err := <-rpcSub.Err(): | |
| l.Debug().Err(err).Msg("client unsubscribed") | |
| case err := <-subs.Err(): | |
| l.Error().Err(err).Msg("subscription error") | |
| } | |
| }() |
janezpodhostnik
force-pushed
the
janez/use-flow-go-components
branch
from
4a8ccb2 to
3e04bc3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
bootstrap/bootstrap.go (3)
521-522: Use structured logging instead of formatted message.Replace
MsgfwithMsgand includecadence_heightas a field for better log parsing.Apply this diff:
logger.Info(). Stringer("fvm_address_for_evm_storage_account", storageAddress). - Msgf("database initialized with cadence height: %d", cadenceHeight) + Uint64("cadence_height", cadenceHeight). + Msg("database initialized")
479-479: Fix typo in variable name.The variable
evmBlokcHeightcontains a typo.Apply this diff:
- evmBlokcHeight := uint64(0) + evmBlockHeight := uint64(0)
207-215: Consider reordering component initialization.The profiler and metrics servers should be initialized first to capture debug data during startup of other components.
Apply this diff:
func (fnb *EVMGatewayNodeBuilder) LoadComponentsAndModules() { fnb.initPublishers() - fnb.Component("Profiler Server", fnb.profilerServerComponent) - fnb.Component("Metrics Server", fnb.metricsServerComponent) - fnb.Component("Key Store", fnb.initKeyStore) - fnb.Component("API Server", fnb.apiServerComponent) - fnb.Component("Event Ingestion Engine", fnb.eventIngestionEngineComponent) + // Initialize profiler and metrics first for debugging + fnb.Component("Metrics Server", fnb.metricsServerComponent) + fnb.Component("Profiler Server", fnb.profilerServerComponent) + // Initialize core components + fnb.Component("Key Store", fnb.initKeyStore) + fnb.Component("API Server", fnb.apiServerComponent) + fnb.Component("Event Ingestion Engine", fnb.eventIngestionEngineComponent)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
bootstrap/bootstrap.go(3 hunks)services/requester/key_store_component.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- services/requester/key_store_component.go
🔇 Additional comments (5)
bootstrap/bootstrap.go (5)
54-78: LGTM! Clean implementation of the node structure.The
EVMGatewayNodeImpstruct andNewNodefunction provide a clear and concise implementation for node management.
585-590: LGTM! Proper cleanup in shutdown function.The shutdown function properly handles the cleanup of the cross-spork client.
384-386:⚠️ Potential issueFix potential integer underflow in missed heights calculation.
The subtraction operation
chainLatestBlock.Height-gatewayLatestHeightcould underflow ifgatewayLatestHeightis greater thanchainLatestBlock.Height.Apply this diff:
- Uint64("missed-heights", chainLatestBlock.Height-gatewayLatestHeight). + Uint64("missed-heights", calculateMissedHeights(chainLatestBlock.Height, gatewayLatestHeight)). // Add helper function: func calculateMissedHeights(chainHeight, gatewayHeight uint64) uint64 { if chainHeight > gatewayHeight { return chainHeight - gatewayHeight } return 0 }Likely invalid or redundant comment.
474-476:⚠️ Potential issueFix improper use of defer with function parameters.
The deferred function is executed immediately due to the function call syntax
(batch). Remove the parameter from the anonymous function.Apply this diff:
- defer func(batch *pebble.Batch) { + defer func() { innerErr = batch.Close() - }(batch) + }()Likely invalid or redundant comment.
462-467: 🛠️ Refactor suggestionAdd validation for force start height.
When force setting the starting Cadence height, there's no validation to ensure the height exists or is valid.
Add validation before setting the force start height:
if cfg.ForceStartCadenceHeight != 0 { logger.Warn().Uint64("height", cfg.ForceStartCadenceHeight).Msg("force setting starting Cadence height!!!") + // Validate the height exists + _, err := fnb.Client.GetBlockHeaderByHeight(context.Background(), cfg.ForceStartCadenceHeight) + if err != nil { + return fmt.Errorf("invalid force start height %d: %w", cfg.ForceStartCadenceHeight, err) + } if err := blocks.SetLatestCadenceHeight(cfg.ForceStartCadenceHeight, nil); err != nil { return err } }✅ Verification successful
The suggested validation is correct and follows existing patterns
The review suggestion is valid as:
- GetBlockHeaderByHeight is the correct API to validate block heights
- Similar validation already exists for normal cadence height in the same file
- The suggested implementation matches existing validation patterns in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for ForceStartCadenceHeight usage and definition rg "ForceStartCadenceHeight" -A 3 # Look for GetBlockHeaderByHeight implementation and usage ast-grep --pattern 'GetBlockHeaderByHeight' # Search for any block height validation patterns rg "GetBlockHeaderByHeight|ValidateHeight|CheckHeight" -A 2Length of output: 3545
| //else { | ||
| // // TODO(JanezP): verify storage account owner is correct | ||
| //} |
There was a problem hiding this comment.
💡 Codebase verification
Storage account owner verification is indeed missing and needs implementation
The concern is validated and particularly important because:
- The register store explicitly delegates owner verification responsibility to callers
- No alternative implementation of this verification exists in the codebase
🔗 Analysis chain
Consider implementing the TODO for storage account verification.
The commented-out TODO suggests a need for storage account owner verification. This could be a security concern if left unimplemented.
Run this script to check if storage account verification is implemented elsewhere:
Would you like me to help implement the storage account verification logic or create a GitHub issue to track this task?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for storage account verification implementations
rg -l "storage.*account.*verif|verif.*storage.*account"
Length of output: 80
Script:
#!/bin/bash
# Get context around the storage account verification in bootstrap.go
rg -B 5 -A 5 "storage.*account.*owner" bootstrap/bootstrap.go
# Also search for general storage account related code
rg -B 5 -A 5 "storage.*account" bootstrap/bootstrap.go
Length of output: 868
Script:
#!/bin/bash
# Search for any related security checks or owner verification patterns
rg -B 3 -A 3 "owner.*verif|verif.*owner"
# Search for any related GitHub issues in comments
rg -B 2 -A 2 "TODO.*storage.*account|TODO.*owner.*verif"
Length of output: 1078
bootstrap/bootstrap.go
Outdated
| l.Info(). | ||
| Uint64("chain-cadence-height", chainLatestBlock.Height). | ||
| Uint64("gateway-cadence-height", gatewayLatestHeight). | ||
| Uint64("missed-heights", chainLatestBlock.Height-gatewayLatestHeight). |
There was a problem hiding this comment.
Simply avoid underflow
| Uint64("missed-heights", chainLatestBlock.Height-gatewayLatestHeight). | |
| Int64("missed-heights", chainLatestBlock.Height-gatewayLatestHeight). |
bootstrap/bootstrap.go
Outdated
| return fmt.Errorf("could not set account status: %w", err) | ||
| } | ||
|
|
||
| err = registerStore.Store(delta.GetUpdates(), evmBlokcHeight, batch) |
There was a problem hiding this comment.
| err = registerStore.Store(delta.GetUpdates(), evmBlokcHeight, batch) | |
| err = registerStore.Store(delta.GetUpdates(), evmBlockHeight, batch) |
|
|
||
| logger.Info(). | ||
| Stringer("fvm_address_for_evm_storage_account", storageAddress). | ||
| Msgf("database initialized with cadence height: %d", cadenceHeight) |
There was a problem hiding this comment.
maybe also log the evm Height
There was a problem hiding this comment.
This is the bootstrapping code branch, so evm height will be 0
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
bootstrap/bootstrap.go (4)
169-179: Enhance error handling in postShutdownThe current implementation collects errors but doesn't provide detailed context about which shutdown functions failed.
Consider enhancing error handling:
-func (fnb *EVMGatewayNodeBuilder) postShutdown() error { +func (fnb *EVMGatewayNodeBuilder) postShutdown() error { var errs *multierror.Error - - for _, fn := range fnb.postShutdownFns { + for i, fn := range fnb.postShutdownFns { err := fn() if err != nil { - errs = multierror.Append(errs, err) + errs = multierror.Append(errs, fmt.Errorf("shutdown function %d failed: %w", i, err)) } } return errs.ErrorOrNil() }
265-272: Improve rate limiter configurationThe current implementation uses math.MaxInt as a fallback which might not be the best approach for disabling rate limiting.
Consider:
- Using a more explicit way to disable rate limiting
- Adding validation for negative rate limits
- Adding configuration for burst limits
rateLimit := cfg.RateLimit if rateLimit == 0 { log.Warn().Msg("no rate-limiting is set") - rateLimit = math.MaxInt + // Use nil limiter for unlimited requests + ratelimiter = memorystore.NoopLimiter{} + return } + if rateLimit < 0 { + return nil, fmt.Errorf("invalid rate limit: %d", rateLimit) + } ratelimiter, err := memorystore.New(&memorystore.Config{ Tokens: rateLimit, Interval: time.Second, + MaxBurst: rateLimit * 2, // Allow bursting up to 2x the rate limit })
367-374: Enhance error handling for block header retrievalThe error message could be more informative about the context and potential solutions.
Consider enhancing the error message:
_, err = fnb.Client.GetBlockHeaderByHeight(context.Background(), latestIndexedHeight) if err != nil { return nil, fmt.Errorf( - "failed to get header for last indexed cadence height %d: %w", + "failed to get header for last indexed cadence height %d. Ensure the access node is synced and the height exists: %w", latestIndexedHeight, err, ) }
503-506: Enhance batch commit error handlingThe batch commit error handling could be more descriptive about the potential causes and recovery steps.
Consider enhancing the error handling:
err = batch.Commit(pebble.Sync) if err != nil { - return fmt.Errorf("could not commit register updates: %w", err) + return fmt.Errorf("failed to commit register updates to database (ensure sufficient disk space and permissions): %w", err) }services/ingestion/event_subscriber.go (3)
79-82: Enhance error context for block header retrievalThe error handling is good but could provide more context about the current height being processed.
Add more context to the error:
- eventsChan <- models.NewBlockEventsError(fmt.Errorf("failed to get latest cadence block: %w", err)) + eventsChan <- models.NewBlockEventsError(fmt.Errorf("failed to get latest cadence block at height %d: %w", r.height, err))
92-97: Enhance logging with additional metricsThe logging is well structured but could benefit from additional metrics to aid in monitoring and debugging.
Add more metrics to the log:
r.logger.Info(). Uint64("chain-cadence-height", latestOnChainHeight). Uint64("latest-indexed-height", r.height). Int64("missed-heights", blocksToCatchUp). + Bool("needs-catchup", blocksToCatchUp > 0). + Str("chain-id", r.chain.String()). Msg("indexing cadence height information")
77-97: Well-structured integration with component modelThe new block header retrieval and status logging integrate well with the component-based architecture mentioned in the PR objectives. The error handling and resource management align with the broader architectural changes.
A few suggestions to enhance the component integration:
- Consider extracting the block header retrieval and status logging into a separate method for better testability
- Add metrics for monitoring component health
Example refactor:
+func (r *RPCEventSubscriber) getAndLogBlockStatus(ctx context.Context) (uint64, error) { + chainLatestBlockHeader, err := r.client.GetLatestBlockHeader(ctx, true) + if err != nil { + return 0, fmt.Errorf("failed to get latest cadence block at height %d: %w", r.height, err) + } + latestOnChainHeight := chainLatestBlockHeader.Height + + blocksToCatchUp := int64(latestOnChainHeight) - int64(r.height) + + r.logger.Info(). + Uint64("chain-cadence-height", latestOnChainHeight). + Uint64("latest-indexed-height", r.height). + Int64("missed-heights", blocksToCatchUp). + Bool("needs-catchup", blocksToCatchUp > 0). + Str("chain-id", r.chain.String()). + Msg("indexing cadence height information") + + return latestOnChainHeight, nil +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
bootstrap/bootstrap.go(3 hunks)services/ingestion/event_subscriber.go(1 hunks)
🔇 Additional comments (3)
bootstrap/bootstrap.go (3)
201-205: Initialize method lacks actual initializationThe Initialize method only prints build details but doesn't perform any actual initialization.
Consider whether this method should:
- Initialize any required resources
- Validate configuration
- Set up any prerequisites
519-521: Implement storage account verificationThe TODO comment indicates missing verification of storage account ownership.
This is a security concern that should be addressed. Would you like me to:
- Generate the implementation for storage account verification
- Create a GitHub issue to track this security enhancement
562-580: LGTM! Well-structured client initializationThe cross-spork client initialization is well-implemented with proper error handling and cleanup.
| blocksToCatchUp := int64(0) | ||
| if latestOnChainHeight > r.height { | ||
| blocksToCatchUp = int64(latestOnChainHeight - r.height) | ||
| } else { | ||
| blocksToCatchUp = int64(r.height - latestOnChainHeight) | ||
| } |
There was a problem hiding this comment.
Fix incorrect calculation of blocks to catch up
The calculation in the else branch is incorrect. When latestOnChainHeight is less than r.height, it means we're ahead of the chain, and there are no blocks to catch up. The current calculation would give a positive number when we should indicate we're ahead.
Apply this fix:
- blocksToCatchUp := int64(0)
- if latestOnChainHeight > r.height {
- blocksToCatchUp = int64(latestOnChainHeight - r.height)
- } else {
- blocksToCatchUp = int64(r.height - latestOnChainHeight)
- }
+ blocksToCatchUp := int64(latestOnChainHeight) - int64(r.height)📝 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.
| blocksToCatchUp := int64(0) | |
| if latestOnChainHeight > r.height { | |
| blocksToCatchUp = int64(latestOnChainHeight - r.height) | |
| } else { | |
| blocksToCatchUp = int64(r.height - latestOnChainHeight) | |
| } | |
| blocksToCatchUp := int64(latestOnChainHeight) - int64(r.height) |
Closes: onflow/flow-go#6776
Depends on: onflow/flow-go#6769
Description
Use flow-go component model and node framework to compose the Gatway and handle shutdown.
Main advantage is that the re-use of the component management system from flow-go automatically gives us nicer error handling an resource closing on error. To use the error handling, just make sure to use
irrecoverable.SignalerContext.Throw(err)for any unexpected error. This will start the shutdown procedure and properly release all resources.Changes:
api/profiler.gois now a componentapi/server.gois now a componentbootstrap/bootstrap.gois changed a lot and is modeled afterflow-goExecutionNodeBuilderbootstrap/utils.gowere moved intoservices/requester/key_store_component.gowhich is a component that handles the keystore. It should have aClosefor the KMS client, but the KMS client is currently not aio.Closer(which should be addresses)cmd/run/cmd.gowas simplified because the startup logic is now actually inflow-gomodels/engine.gowas removed because its no longer neededmodels/stream.gowas converted into a component. This lets it force shutdown the gateway in case of an error inNotifyservices/ingestion/engine.gois now component. In case of errors in events processing it shuts down the node.This depends on onflow/flow-go#6769
For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
Release Notes
Architecture Refactoring
Dependency Updates
v0.38.0-preview.0.4.0.20250102180624-72adf9e522c4.Performance Improvements
Testing Enhancements
Cleanup
Engine.