Track & apply surge-factor pricing changes #867
Conversation
WalkthroughAdds surge-aware fee handling: new FeeParameters model, indexer (interface, pebble impl, mock), event decoding/storage for FlowFees.FeeParametersChanged, ingestion and bootstrap wiring, requester/API plumbing to compute/enforce surge-adjusted gas prices, tests, and a Cadence script to read network surge factor. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Flow as Flow Network
participant Sub as EventSubscriber
participant Ing as IngestionEngine
participant Store as FeeParametersStore
Note over Flow,Sub: Blocks/events may include FlowFees.FeeParametersChanged
Flow->>Sub: event stream (incl. FeeParametersChanged)
Sub->>Ing: CadenceEvents (parsed)
alt FeeParametersChanged present
Ing->>Store: Store(feeParameters, batch)
Store-->>Ing: ok
else no update
Note over Ing: continue ingestion
end
sequenceDiagram
autonumber
participant Client
participant API as BlockChainAPI
participant Store as FeeParametersStore
participant Req as Requester/EVM
Client->>API: eth_sendRawTransaction(data)
API->>Store: Get()
alt fee params ok
Store-->>API: FeeParameters
API->>Req: SendRawTransaction(data, feeParams)
Req-->>API: txHash
API-->>Client: txHash
else store error
Store-->>API: error
API-->>Req: SendRawTransaction(data, defaultFeeParams)
Req-->>API: txHash/error
API-->>Client: txHash/error
end
sequenceDiagram
autonumber
participant Client
participant API as BlockChainAPI
participant Store as FeeParametersStore
Client->>API: eth_gasPrice / eth_maxPriorityFeePerGas / eth_feeHistory
API->>Store: Get()
alt ok
Store-->>API: FeeParameters
API-->>Client: CalculateGasPrice(base) using SurgeFactor
else error
API-->>Client: base (fallback)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
m-Peter
force-pushed
the
mpeter/track-surge-factor
branch
4 times, most recently
from
ba80db8 to
194c627
Compare
m-Peter
changed the title
m-Peter
force-pushed
the
mpeter/track-surge-factor
branch
from
194c627 to
60fc001
Compare
m-Peter
requested review from
janezpodhostnik,
peterargue and
zhangchiqing
as code owners
![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
🧹 Nitpick comments (12)
storage/pebble/keys.go (1)
30-32: New key namespace is fine; consider documenting reserved rangesAdding feeParametersKey = byte(60) does not collide with existing keys and stays outside the register key marker (50). Consider adding a short comment in keys.go or a storage README about reserved ranges to avoid accidental reuse in future keys.
tests/e2e_web3js_test.go (1)
118-156: Fix typo in test name: “multipler” → “multiplier”Minor naming nit to keep the test list polished.
Apply this diff:
- t.Run("gas price updated with surge factor multipler", func(t *testing.T) { + t.Run("gas price updated with surge factor multiplier", func(t *testing.T) {tests/web3js/eth_gas_price_surge_test.js (3)
13-13: Nit: keep method name casing consistent in descriptionThe RPC method is eth_maxPriorityFeePerGas (lower-case m). Purely cosmetic, but keeps test names consistent with RPC names.
-it('should update the value of eth_MaxPriorityFeePerGas', async () => { +it('should update the value of eth_maxPriorityFeePerGas', async () => {
83-84: Typo in test title: “eth_feeFistory” → “eth_feeHistory”Cosmetic fix for clarity.
-it('should update gas price for eth_feeFistory', async () => { +it('should update gas price for eth_feeHistory', async () => {
85-86: Remove debugging console.log to keep CI logs cleanDrop the log to reduce noise in CI output.
- console.log('Response: ', response)services/ingestion/engine.go (1)
223-231: Consider adding metrics for fee parameter updatesThe fee parameter storage logic is correctly placed before the heartbeat handling. However, for better observability, consider adding metrics to track fee parameter updates similar to how block and transaction indexing is tracked.
Consider tracking fee parameter updates with metrics for better monitoring:
if events.FeeParameters() != nil { if err := e.feeParameters.Store(events.FeeParameters(), batch); err != nil { return fmt.Errorf( "failed to update fee parameters for height: %d, during events ingestion: %w", events.CadenceHeight(), err, ) } + // Track fee parameter updates for monitoring + e.collector.FeeParametersIndexed(events.CadenceHeight()) }api/api.go (2)
185-188: Add error handling context for fee parameters retrievalWhile the error handling is correct, consider wrapping the error with more context to help with debugging in production.
feeParams, err := b.feeParameters.GetLatest() if err != nil { - return common.Hash{}, err + return common.Hash{}, fmt.Errorf("failed to get latest fee parameters: %w", err) }Also applies to: 190-190
1022-1028: Consider caching fee parameters for frequently called endpointsThe
GasPriceandMaxPriorityFeePerGasendpoints may be called frequently. Consider implementing a short-lived cache for the latest fee parameters to reduce database lookups.Consider implementing a simple cache with a short TTL (e.g., 1-2 seconds) for the latest fee parameters to improve performance of these frequently called endpoints. This would reduce database pressure while still maintaining relatively up-to-date pricing information.
Also applies to: 1069-1076
services/requester/requester.go (1)
170-174: Method signature updated consistently; ensure upstream always passes feeParams.The EVM method mirrors the interface change. Given it dereferences feeParams later, add a local nil fallback to avoid a nil deref if upstream forgets to pass it.
models/fee_parameters.go (2)
15-20: Sensible defaults.Default surge and effort costs set to 1x with EffectiveEVMHeight=1 ensure sane behavior at bootstrap. Consider documenting feeParamsPrecision briefly.
47-58: Event decode: consider validating invariants.After decoding, optionally validate EffectiveEVMHeight > 0 and that surge/effort costs are non-zero to avoid storing malformed parameters from unexpected events.
storage/pebble/fee_parameters.go (1)
27-45: Append-only storage strategy: consider monotonicity/dedup safeguards.If events arrive out of order or duplicates at the same EffectiveEVMHeight appear, reverse-iteration lookups may return incorrect parameters. Consider either:
- enforcing non-decreasing EffectiveEVMHeight on Store (reject or overwrite), or
- inserting in order and deduping by EffectiveEVMHeight.
Example guard:
func (f *FeeParameters) Store(fp *models.FeeParameters, batch *pebble.Batch) error { feeParams, err := f.getAll() if err != nil { return err } + n := len(feeParams) + if n > 0 { + last := feeParams[n-1] + if fp.EffectiveEVMHeight < last.EffectiveEVMHeight { + return fmt.Errorf("fee parameters height %d older than last stored %d", fp.EffectiveEVMHeight, last.EffectiveEVMHeight) + } + if fp.EffectiveEVMHeight == last.EffectiveEVMHeight { + // overwrite last to dedupe + feeParams[n-1] = fp + } else { + feeParams = append(feeParams, fp) + } + } else { + feeParams = append(feeParams, fp) + } - feeParams = append(feeParams, feeParameters)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
Makefile(1 hunks)api/api.go(7 hunks)bootstrap/bootstrap.go(5 hunks)models/events.go(6 hunks)models/fee_parameters.go(1 hunks)services/ingestion/engine.go(4 hunks)services/ingestion/engine_test.go(10 hunks)services/ingestion/event_subscriber.go(3 hunks)services/requester/requester.go(3 hunks)storage/index.go(1 hunks)storage/mocks/FeeParametersIndexer.go(1 hunks)storage/pebble/fee_parameters.go(1 hunks)storage/pebble/keys.go(1 hunks)tests/e2e_web3js_test.go(2 hunks)tests/web3js/eth_gas_price_surge_test.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-18T19:26:37.579Z
Learnt from: peterargue
PR: onflow/flow-evm-gateway#617
File: api/stream.go:62-67
Timestamp: 2024-10-18T19:26:37.579Z
Learning: In the `flow-evm-gateway` project, within the Go file `api/stream.go`, the `prepareBlockResponse` method includes the Bloom filter as the field `LogsBloom` in the returned `Block` struct.
Applied to files:
services/ingestion/event_subscriber.go
🧬 Code graph analysis (14)
storage/index.go (2)
storage/pebble/fee_parameters.go (1)
FeeParameters(17-19)models/fee_parameters.go (1)
FeeParameters(22-27)
tests/web3js/eth_gas_price_surge_test.js (1)
tests/e2e-network/e2e_test.js (1)
assert(3-3)
storage/pebble/keys.go (1)
storage/pebble/register_storage.go (1)
newLookupKey(151-179)
tests/e2e_web3js_test.go (1)
tests/helpers.go (1)
setupTestAccounts(231-287)
models/fee_parameters.go (2)
storage/pebble/fee_parameters.go (1)
FeeParameters(17-19)models/transaction.go (2)
dc(109-111)dc(117-119)
services/requester/requester.go (2)
models/fee_parameters.go (1)
FeeParameters(22-27)models/errors/errors.go (1)
NewTxGasPriceTooLowError(61-66)
services/ingestion/engine.go (3)
storage/index.go (1)
FeeParametersIndexer(106-112)storage/pebble/fee_parameters.go (1)
FeeParameters(17-19)models/fee_parameters.go (1)
FeeParameters(22-27)
storage/mocks/FeeParametersIndexer.go (2)
storage/index.go (1)
FeeParametersIndexer(106-112)models/fee_parameters.go (1)
FeeParameters(22-27)
api/api.go (1)
storage/index.go (1)
FeeParametersIndexer(106-112)
services/ingestion/event_subscriber.go (1)
models/events.go (1)
FeeParametersChangedQualifiedIdentifier(18-18)
services/ingestion/engine_test.go (2)
storage/index.go (1)
FeeParametersIndexer(106-112)storage/mocks/FeeParametersIndexer.go (1)
FeeParametersIndexer(13-15)
models/events.go (1)
models/fee_parameters.go (1)
FeeParameters(22-27)
bootstrap/bootstrap.go (4)
storage/index.go (1)
FeeParametersIndexer(106-112)storage/pebble/fee_parameters.go (2)
FeeParameters(17-19)NewFeeParameters(21-25)models/fee_parameters.go (2)
FeeParameters(22-27)DefaultFeeParameters(15-20)models/errors/errors.go (1)
ErrEntityNotFound(38-38)
storage/pebble/fee_parameters.go (5)
storage/index.go (1)
FeeParametersIndexer(106-112)models/fee_parameters.go (1)
FeeParameters(22-27)storage/pebble/storage.go (1)
Storage(13-16)models/errors/errors.go (1)
ErrEntityNotFound(38-38)storage/pebble/register_storage.go (1)
Store(84-100)
🔇 Additional comments (17)
Makefile (1)
103-103: Add FeeParametersIndexer to mocks: LGTMThe new mockery target for FeeParametersIndexer aligns with the new interface in storage/index.go and the mock at storage/mocks/FeeParametersIndexer.go. No issues spotted.
tests/e2e_web3js_test.go (1)
6-6: Import of fmt is required and correctly usedfmt is needed for the fmt.Sprintf Cadence script below. Looks good.
services/ingestion/engine_test.go (1)
57-57: LGTM! Consistent test setup with new FeeParametersIndexer mockThe addition of the
feeParamsmock and its inclusion inNewEventIngestionEngineconstructor is properly implemented across all test cases, maintaining consistency with the new API changes.Also applies to: 77-77
services/ingestion/engine.go (1)
51-51: LGTM! Properly integrated FeeParametersIndexer dependencyThe new field and constructor parameter are well-integrated into the Engine struct, following the existing pattern for other storage indexers.
Also applies to: 69-69, 89-89
models/events.go (3)
37-44: LGTM! Clean implementation of fee parameters event detectionThe implementation follows the established pattern for event detection functions and properly validates the event type before checking the qualified identifier.
139-146: Error handling fordecodeFeeParametersChangedEventhas been verifiedThe
decodeFeeParametersChangedEventfunction inmodels/fee_parameters.gocorrectly propagates decoding errors by wrapping them with context using Go’s%wverb and returning immediately. There are no specialized error types for fee-parameter decoding, and this pattern is consistent with other event decoders in the codebase.•
cadence.DecodeFieldserror is caught and wrapped with a descriptive message
• Function returnsnil, erroron failure and*FeeParameters, nilon successNo further changes are required here.
172-174: Ignore thread-safety concern for EffectiveEVMHeight assignmentThe
*FeeParametersinstance is freshly allocated bydecodeFeeParametersChangedEventfor each event and stored only in the localCadenceEventsobject. It’s never shared across goroutines before being persisted (viaEngine.indexEvents) or returned by storage as a new copy. There are nogo funcusages capturing the sameFeeParameterspointer, and all mutations (including settingEffectiveEVMHeight) occur in the same execution thread. You can safely disregard this suggestion.Likely an incorrect or invalid review comment.
services/ingestion/event_subscriber.go (3)
562-576: LGTM! Well-structured event filter compositionThe implementation cleanly separates EVM events from fee parameter events and properly composes them into a single filter. The documentation clearly explains the subscription scope.
567-571: Verify FlowFees contract address is provided for every supported chainThe code at
services/ingestion/event_subscriber.gounconditionally uses the FlowFees address returned bycontracts := systemcontracts.SystemContractsForChain(chainID) flowFeesAddress := common.Address(contracts.FlowFees.Address) feeParametersChangedEvent := common.NewAddressLocation( nil, flowFeesAddress, models.FeeParametersChangedQualifiedIdentifier, ).ID()But if the external
systemcontractspackage doesn’t define a FlowFees address for a givenchainID, this will silently yield a zero address and may break event filtering without an obvious error. Since we can’t inspect that module here, please manually verify that:
SystemContractsForChain(chainID)includes a non-zeroFlowFees.Addressfor all chain IDs your service supports.- If any chain is missing this contract, you add a guard, e.g.:
contracts := systemcontracts.SystemContractsForChain(chainID) if contracts.FlowFees.Address == (common.Address{}) { return nil, fmt.Errorf("FlowFees contract not found for chain %s", chainID) }• File: services/ingestion/event_subscriber.go
• Lines: 563–567
474-474: fetchMissingData only recovers EVM block/transaction events—not fee parameter changesIn services/ingestion/event_subscriber.go (around line 474),
fetchMissingDataiterates overevmEventFilter(r.chain).EventTypes—but
evmEventFilterreturns only the EVM block and transaction event types. The function that includes fee‐parameter changes,blocksEventFilter, is never used here. As a result, anyFlowFees.FeeParametersChangedevents will not be fetched during the fallback recovery.Action items:
- If you need to recover fee‐parameter changes in fallback mode, update
fetchMissingDatato useblocksEventFilter(r.chain).EventTypes(or explicitly append the fee‐parameter event ID).- Otherwise, add a comment or docs explaining that fee parameter events are intentionally excluded from the recovery path and that their absence in fallback mode is acceptable.
bootstrap/bootstrap.go (4)
54-61: New storage dependency wired correctly.Adding FeeParameters to Storages is coherent with the broader PR wiring (ingestion + API). No issues spotted.
195-196: Event ingestion now receives FeeParameters indexer; LGTM.This ensures FeeParametersChanged events can be persisted and queried. Good placement in the constructor args.
319-320: API wiring passes FeeParameters to BlockChainAPI; LGTM.This will allow eth_gasPrice and eth_feeHistory to reflect surge pricing as intended.
666-673: Return struct includes FeeParameters; LGTM.Good to expose through Storages for both API and ingestion wiring.
storage/mocks/FeeParametersIndexer.go (1)
1-108: Mock looks correct and matches the interface.Auto-generated mock aligns with storage.FeeParametersIndexer. No issues.
storage/pebble/fee_parameters.go (2)
61-74: Lookup strategy LGTM given expected cardinality.Reverse scan by EffectiveEVMHeight is acceptable here. If the list grows large, consider indexing by height range, but that’s likely premature.
76-91: Error handling and not-found behavior LGTM.Returning an empty slice on not found simplifies callers. Decode error wrapping is helpful for diagnosis.
m-Peter
force-pushed
the
mpeter/track-surge-factor
branch
from
e188bf0 to
557d5f4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
api/api.go (1)
832-841: FeeHistory: add visibility, avoid per-iteration Get(), and fix percentile loop shadowing
- Log when fee parameters cannot be retrieved (visibility). This echoes a prior review; keeping it here due to impact.
- Calling Get() for every block is redundant if you intentionally use “current” params for the whole window; fetch once per request.
- Minor: rename inner loop index to avoid shadowing the outer i.
Minimal logging + loop variable rename:
- blockRewards := make([]*hexutil.Big, len(rewardPercentiles)) - feeParams, err := b.feeParameters.Get() - if err != nil { - continue - } - gasPrice := feeParams.CalculateGasPrice(b.config.GasPrice) - for i := range rewardPercentiles { - blockRewards[i] = (*hexutil.Big)(gasPrice) - } + blockRewards := make([]*hexutil.Big, len(rewardPercentiles)) + feeParams, err := b.feeParameters.Get() + if err != nil { + b.logger.Warn(). + Uint64("height", blockHeight). + Err(err). + Msg("failed to get fee parameters for block in fee history") + continue + } + gasPrice := feeParams.CalculateGasPrice(b.config.GasPrice) + for p := range rewardPercentiles { + blockRewards[p] = (*hexutil.Big)(gasPrice) + }Optional, fetch params once per request (outside the loop) if “latest applies to all blocks” is the intended semantics:
// place before the for-loop feeParams, fpErr := b.feeParameters.Get() var gasPrice *big.Int if fpErr != nil { b.logger.Warn().Err(fpErr).Msg("fee parameters unavailable; feeHistory will use base gas price") gasPrice = b.config.GasPrice } else { gasPrice = feeParams.CalculateGasPrice(b.config.GasPrice) } // inside the loop, just use gasPrice computed aboveOptional, make oldestBlock resilient if early blocks are missing (today it can remain nil when the first block fails):
// after successfully fetching 'block' if oldestBlock == nil { oldestBlock = (*hexutil.Big)(big.NewInt(int64(block.Height))) }If historical accuracy is required (surge factor may change within the window), consider extending the indexer with GetAtHeight(height uint64) and using that here; otherwise document that the current surge factor is used across the returned range.
🧹 Nitpick comments (4)
api/api.go (4)
185-191: Use consistent error handling/logging for fee parameter retrieval in SendRawTransactionCurrently you return the raw error; other paths use handleError to normalize metrics/logs. Align for observability.
- feeParams, err := b.feeParameters.Get() - if err != nil { - return common.Hash{}, err - } + feeParams, err := b.feeParameters.Get() + if err != nil { + return handleError[common.Hash](err, l, b.collector) + }
780-854: Consider preallocating FeeHistory slices for performanceSince maxCount is known, preallocate baseFees, rewards, and gasUsedRatios with cap=maxCount to reduce allocations.
Example (outside the selected lines for context):
baseFees = make([]*hexutil.Big, 0, maxCount) rewards = make([][]*hexutil.Big, 0, maxCount) gasUsedRatios = make([]float64, 0, maxCount)
1020-1029: DRY: factor out “current surge-adjusted gas price” helperBoth endpoints share identical logic. A small helper improves readability and testability.
Example helper (outside the selected lines):
func (b *BlockChainAPI) currentGasPriceOrBase() *big.Int { fp, err := b.feeParameters.Get() if err != nil { b.logger.Warn().Err(err).Msg("fee parameters unavailable; using base gas price") return b.config.GasPrice } return fp.CalculateGasPrice(b.config.GasPrice) }Then:
func (b *BlockChainAPI) GasPrice(ctx context.Context) (*hexutil.Big, error) { return (*hexutil.Big)(b.currentGasPriceOrBase()), nil } func (b *BlockChainAPI) MaxPriorityFeePerGas(ctx context.Context) (*hexutil.Big, error) { return (*hexutil.Big)(b.currentGasPriceOrBase()), nil }If you want, I can push a small follow-up PR with this refactor and tests.
Also applies to: 1067-1076
780-854: Add tests covering surge-factor application and fallbacks
- eth_gasPrice and eth_maxPriorityFeePerGas: verify base → surge-adjusted after FeeParametersChanged ingestion; verify fallback when index returns error.
- eth_feeHistory: verify reward values reflect surge-adjusted price (or base on fallback) and oldestBlock semantics when early blocks are missing.
I can draft table-driven tests with mocked FeeParametersIndexer producing (a) values, (b) errors, and with varying rewardPercentiles.
Also applies to: 1020-1029, 1067-1076
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
api/api.go(7 hunks)tests/e2e_web3js_test.go(2 hunks)tests/web3js/eth_gas_price_surge_test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e_web3js_test.go
- tests/web3js/eth_gas_price_surge_test.js
🧰 Additional context used
🧬 Code graph analysis (1)
api/api.go (1)
storage/index.go (1)
FeeParametersIndexer(106-110)
⏰ 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 (2)
api/api.go (2)
88-92: New dependency injection looks goodAdding FeeParametersIndexer to the API surface is the right direction for surge-aware pricing.
101-118: Add nil-check for feeParameters in NewBlockChainAPIThe constructor now enforces a non-nil
feeParametersand will fail fast if it’s missing:func NewBlockChainAPI( logger zerolog.Logger, config config.Config, evm requester.Requester, blocks storage.BlockIndexer, transactions storage.TransactionIndexer, receipts storage.ReceiptIndexer, feeParameters storage.FeeParametersIndexer, rateLimiter RateLimiter, collector metrics.Collector, indexingResumedHeight uint64, ) *BlockChainAPI { + if feeParameters == nil { + logger.Fatal().Msg("fee parameters indexer is required for surge-aware pricing") + } return &BlockChainAPI{ logger: logger, config: config, evm: evm, blocks: blocks, transactions: transactions, receipts: receipts, feeParameters: feeParameters, indexingResumedHeight: indexingResumedHeight, rateLimiter: rateLimiter, collector: collector, } }• Verified that the only call site for
NewBlockChainAPIis inbootstrap/bootstrap.goand it passes the newfeeParametersargument.
• No other usages were found in the codebase or test files.With this guard in place and all constructors updated, we can resolve this comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
models/fee_parameters.go (1)
31-40: Follow-up: past overflow concern is resolvedThis implementation addresses the earlier overflow/precision issues flagged in previous rounds by keeping the entire computation in big.Int and guarding nil.
🧹 Nitpick comments (8)
models/fee_parameters.go (4)
31-40: Big-int math + nil guard: LGTM; consider rounding directionThe switch to big.Int arithmetic and the nil check address overflow/truncation risks. One follow-up: integer division floors the result, which can slightly understate the suggested price. If you prefer “never underquote,” use ceil division.
Apply this diff to round up:
func (f *FeeParameters) CalculateGasPrice(currentGasPrice *big.Int) *big.Int { if currentGasPrice == nil { return new(big.Int) // zero } // gasPrice = (currentGasPrice * surgeFactor) / feeParamsPrecision - surgeFactor := new(big.Int).SetUint64(uint64(f.SurgeFactor)) - gasPrice := new(big.Int).Mul(currentGasPrice, surgeFactor) - return new(big.Int).Quo(gasPrice, surgeFactorScale) + sf := new(big.Int).SetUint64(uint64(f.SurgeFactor)) + num := new(big.Int).Mul(currentGasPrice, sf) + // ceil division: (num + denom - 1) / denom + num.Add(num, new(big.Int).Sub(surgeFactorScale, big.NewInt(1))) + return new(big.Int).Quo(num, surgeFactorScale) }
27-30: Guard against nil receiver in ToBytesMinor safety improvement to avoid accidental panics if ToBytes is ever called on a nil pointer.
Apply this diff:
func (f *FeeParameters) ToBytes() ([]byte, error) { - return rlp.EncodeToBytes(f) + if f == nil { + return nil, fmt.Errorf("nil FeeParameters") + } + return rlp.EncodeToBytes(f) }
51-62: Defensive check: validate event type before decodingIf this helper is ever called with a non-FeeParametersChanged event, DecodeFields may succeed unexpectedly or error with less context. Validate the qualified identifier up front.
Apply this diff:
func decodeFeeParametersChangedEvent(event cadence.Event) (*FeeParameters, error) { + if event.EventType.QualifiedIdentifier() != FeeParametersChangedQualifiedIdentifier { + return nil, fmt.Errorf("unexpected event type %q, expected %q", + event.EventType.QualifiedIdentifier(), FeeParametersChangedQualifiedIdentifier) + } feeParameters := &FeeParameters{} if err := cadence.DecodeFields(event, feeParameters); err != nil { return nil, fmt.Errorf( "failed to Cadence-decode FlowFees.FeeParametersChanged event [%s]: %w", event.String(), err, ) } return feeParameters, nil }
15-19: Defaults look correct; add a brief comment on units/scaleTiny doc-nit: note that these are UFix64 fixed-point values scaled by 1e8 so readers don’t have to hunt for feeParamsPrecision semantics.
api/api.go (4)
185-191: Avoid hard-failing SendRawTransaction on transient fee-parameter unavailabilityFalling back preserves submit availability and aligns with GasPrice/MaxPriority behavior. Use DefaultFeeParameters (or a documented base) with a warning.
Apply this diff:
- feeParams, err := b.feeParameters.Get() - if err != nil { - return common.Hash{}, err - } + feeParams, err := b.feeParameters.Get() + if err != nil { + b.logger.Warn(). + Err(err). + Msg("fee parameters unavailable; falling back to default for tx submission") + feeParams = models.DefaultFeeParameters + } id, err := b.evm.SendRawTransaction(ctx, input, feeParams)
1027-1033: GasPrice fallback: LGTMGraceful degradation keeps wallets functional when fee params blip. Consider emitting a metric to track fallback usage rate.
176-179: Avoid logging full raw tx hex on errorsLogging the entire signed tx input can leak user data pre-broadcast and grows logs. Prefer logging size and/or hash.
Apply this diff:
- l := b.logger.With(). - Str("endpoint", EthSendRawTransaction). - Str("input", input.String()). - Logger() + l := b.logger.With(). + Str("endpoint", EthSendRawTransaction). + Int("input_len_bytes", len(input)). + Logger()Optionally include the keccak hash if useful:
// h := common.BytesToHash(crypto.Keccak256(input)) // .Str("input_hash", h.Hex())
780-859: Micro-optimization: fetch fee parameters once per response (optional)Get() inside the loop can return different values mid-iteration and costs repeated lookups. If the spec is “reflect current surge,” fetch once before the loop and reuse; if it’s “historically accurate,” add a GetAtHeight(height) API and use it here.
Would you like me to draft a FeeParametersIndexer.GetAtHeight(height uint64) extension and wire it through Pebble? I can also add tests to validate per-height behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
api/api.go(7 hunks)bootstrap/bootstrap.go(5 hunks)models/fee_parameters.go(1 hunks)tests/web3js/eth_gas_price_surge_test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/web3js/eth_gas_price_surge_test.js
- bootstrap/bootstrap.go
🧰 Additional context used
🧬 Code graph analysis (2)
api/api.go (1)
storage/index.go (1)
FeeParametersIndexer(106-110)
models/fee_parameters.go (1)
storage/pebble/fee_parameters.go (1)
FeeParameters(14-17)
⏰ 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 (3)
api/api.go (3)
88-89: New field injection: LGTMAdding feeParameters to BlockChainAPI keeps the concern localized and testable.
1074-1080: MaxPriorityFeePerGas fallback: LGTMMirrors GasPrice behavior for consistency across pricing endpoints.
101-118: Ensure NewBlockChainAPI calls and tests include the newfeeParametersdependencyThe
NewBlockChainAPIconstructor signature now expects afeeParameters storage.FeeParametersIndexerargument. Please verify that:
- Every call site (currently only in bootstrap/bootstrap.go around line 312) passes
b.feeParametersin the correct position betweenreceiptsandrateLimiter.- Any tests or mock implementations that instantiate
BlockChainAPI(e.g. calls toNewBlockChainAPIin_test.gofiles or in yourmocks/directory) have been updated to supply the newfeeParametersargument.No other invocation sites were found via
rg -nP '\bNewBlockChainAPI\s*\('—double-check tests/mocks to avoid compilation errors.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
services/ingestion/event_subscriber.go (1)
555-582: Centralize EVM event ID construction
- Replace ad-hoc
common.NewAddressLocation(nil, evmAddr, string(events.EventType…)).ID()in bothevmEventFilterdefinitions (around lines 297–306 and 555–574) with the newevmEventIDsForChain(chainID)helper.- Refactor the inline address/location logic in
backfillSporkFromHeight(around line 295) to useids := evmEventIDsForChain(r.chain)andids.BlockExecuted/ids.TransactionExecuted.- Verify that the EVM system contract address does not change at spork boundaries; if it can, derive event IDs per spork (e.g., via CrossSporkClient metadata) to avoid missed events.
services/ingestion/engine.go (1)
128-166: Don’t stop the engine when the fee-params stream closesOn feeParamsEvents close you return nil, terminating ingestion even if EVM events continue. Null out the channel instead, as we do with other optional streams.
Apply:
- case feeParams, ok := <-feeParamsEvents: - if !ok { - return nil - } + case feeParams, ok := <-feeParamsEvents: + if !ok { + // stop listening to fee-params but keep ingesting EVM events + feeParamsEvents = nil + continue + }
♻️ Duplicate comments (1)
bootstrap/bootstrap.go (1)
659-667: Good: propagate unexpected storage errors; consider logging when bootstrapping defaultsYou implemented the earlier recommendation to fail fast on non-ErrEntityNotFound. Add an info log when storing defaults to aid ops visibility.
Apply:
- if _, err = feeParameters.Get(); errors.Is(err, errs.ErrEntityNotFound) { + if _, err = feeParameters.Get(); errors.Is(err, errs.ErrEntityNotFound) { + logger.Info().Msg("bootstrapping default fee parameters") if err := feeParameters.Store(models.DefaultFeeParameters, batch); err != nil { return nil, nil, fmt.Errorf("failed to bootstrap fee parameters: %w", err) }
🧹 Nitpick comments (5)
services/ingestion/event_subscriber.go (1)
474-496: Avoid implicit dependence on filter order; use named event IDs.Future additions to evmEventFilter could silently change behavior here. Iterate explicit, named IDs to make intent stable.
Apply this diff:
- for _, eventType := range evmEventFilter(r.chain).EventTypes { + ids := evmEventIDsForChain(r.chain) + for _, eventType := range []string{ids.BlockExecuted, ids.TransactionExecuted} { recoveredEvents, err := r.client.GetEventsForHeightRange( ctx, eventType, blockEvents.Height, blockEvents.Height, )services/ingestion/engine_test.go (1)
68-73: Add a test that exercises fee-params ingestion pathYou inject a FeeParamsSubscriber but never emit a FeeParametersChanged event nor assert feeParams.Store is called. Add a channel you control, send a populated FeeParamsEvents, and assert the indexer’s Store is invoked.
I can draft the test update if helpful.
services/ingestion/fee_params_subscriber.go (3)
72-74: Fix misleading comment (“EVM specific events”).This subscriber listens for FlowFees.FeeParametersChanged, not EVM-specific events.
-// Subscribing to EVM specific events and handle any disconnection errors +// Subscribe to FlowFees fee-parameter change events and handle disconnections
125-135: Deduplicate “stream closed” handling.The two branches that handle closed streams/errChan are identical. Factor into a tiny helper to reduce repetition and keep behavior consistent.
@@ - case blockEvents, ok := <-blockEventsStream: + case blockEvents, ok := <-blockEventsStream: if !ok { - // typically we receive an error in the errChan before the channels are closed - var err error - err = errs.ErrDisconnected - if ctx.Err() != nil { - err = ctx.Err() - } - eventsChan <- models.NewFeeParamsEventsError(err) - return + sendDisconnected := func() { + err := errs.ErrDisconnected + if ctx.Err() != nil { + err = ctx.Err() + } + eventsChan <- models.NewFeeParamsEventsError(err) + } + sendDisconnected() + return } @@ - case err, ok := <-errChan: + case err, ok := <-errChan: if !ok { - // typically we receive an error in the errChan before the channels are closed - var err error - err = errs.ErrDisconnected - if ctx.Err() != nil { - err = ctx.Err() - } - eventsChan <- models.NewFeeParamsEventsError(err) - return + sendDisconnected() + return }Also applies to: 145-154
52-54: Optional: extract channel buffer size to a named const or config.Makes tuning easier and documents intent.
- eventsChan := make(chan *models.FeeParamsEvents, 1000) + const feeParamsBuf = 1000 // TODO: move to config if needed + eventsChan := make(chan *models.FeeParamsEvents, feeParamsBuf)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
bootstrap/bootstrap.go(5 hunks)models/events.go(3 hunks)services/ingestion/engine.go(5 hunks)services/ingestion/engine_test.go(9 hunks)services/ingestion/event_subscriber.go(3 hunks)services/ingestion/fee_params_subscriber.go(1 hunks)services/ingestion/mocks/FeeParamsSubscriber.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-18T19:26:37.579Z
Learnt from: peterargue
PR: onflow/flow-evm-gateway#617
File: api/stream.go:62-67
Timestamp: 2024-10-18T19:26:37.579Z
Learning: In the `flow-evm-gateway` project, within the Go file `api/stream.go`, the `prepareBlockResponse` method includes the Bloom filter as the field `LogsBloom` in the returned `Block` struct.
Applied to files:
services/ingestion/event_subscriber.go
🧬 Code graph analysis (6)
services/ingestion/fee_params_subscriber.go (4)
models/events.go (5)
FeeParamsEvents(268-271)NewFeeParamsEventsError(288-292)BlockEvents(218-221)NewFeeParamsEvents(273-286)FeeParametersChangedQualifiedIdentifier(18-18)services/requester/cross-spork_client.go (1)
CrossSporkClient(108-113)models/errors/errors.go (1)
ErrDisconnected(24-24)services/ingestion/event_subscriber_test.go (1)
Test_Subscribing(31-75)
services/ingestion/mocks/FeeParamsSubscriber.go (3)
services/ingestion/fee_params_subscriber.go (1)
FeeParamsSubscriber(23-25)models/events.go (1)
FeeParamsEvents(268-271)services/ingestion/mocks/EventSubscriber.go (2)
NewEventSubscriber(40-50)Subscribe(19-36)
models/events.go (1)
models/fee_parameters.go (1)
FeeParameters(21-25)
bootstrap/bootstrap.go (6)
storage/pebble/storage.go (1)
Storage(13-16)storage/index.go (5)
BlockIndexer(12-59)TransactionIndexer(84-95)ReceiptIndexer(61-82)TraceIndexer(97-104)FeeParametersIndexer(106-110)models/fee_parameters.go (2)
FeeParameters(21-25)DefaultFeeParameters(15-19)storage/pebble/fee_parameters.go (2)
FeeParameters(14-17)NewFeeParameters(19-23)services/ingestion/fee_params_subscriber.go (1)
NewFeeParamsEventSubscriber(36-49)services/ingestion/engine.go (1)
NewEventIngestionEngine(61-99)
services/ingestion/engine_test.go (4)
storage/index.go (1)
FeeParametersIndexer(106-110)services/ingestion/fee_params_subscriber.go (1)
FeeParamsSubscriber(23-25)models/events.go (1)
FeeParamsEvents(268-271)services/ingestion/engine.go (1)
NewEventIngestionEngine(61-99)
services/ingestion/engine.go (6)
services/ingestion/event_subscriber.go (1)
EventSubscriber(27-33)services/ingestion/fee_params_subscriber.go (1)
FeeParamsSubscriber(23-25)storage/index.go (1)
FeeParametersIndexer(106-110)models/events.go (1)
FeeParamsEvents(268-271)models/fee_parameters.go (1)
FeeParameters(21-25)storage/pebble/fee_parameters.go (1)
FeeParameters(14-17)
⏰ 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 (14)
services/ingestion/event_subscriber.go (1)
146-151: Switch to evmEventFilter looks good.The subscription now targets the exact EVM events via address-scoped IDs; consistent with the rest of the file.
models/events.go (3)
16-19: Constant addition looks goodIdentifier string matches expected fully-qualified Cadence event name.
37-44: Event-type guard LGTMNil-safe and consistent with existing helpers.
288-292: Helper constructor LGTMSimple, consistent with other New*Error helpers.
services/ingestion/engine.go (2)
43-59: Engine fields extended appropriatelyNew dependencies are injected via constructor; consistent with existing style.
61-99: Constructor wiring LGTMProperly assigns new fields and keeps logging scope.
services/ingestion/mocks/FeeParamsSubscriber.go (1)
13-50: Generated mock looks correctMatches interface signature; cleanup assertion wired.
bootstrap/bootstrap.go (4)
54-61: Storages struct extension LGTMField naming and placement are consistent with other indexers.
185-191: Subscriber wiring LGTMStarts FeeParamsEventSubscriber at the same cadence height as EVM events.
192-209: Engine constructor updates LGTMCorrect parameter order matching engine.NewEventIngestionEngine.
320-331: API wiring LGTMFeeParameters passed into API; aligns with surge-aware endpoints.
services/ingestion/fee_params_subscriber.go (3)
190-205: LGTM: correct, chain-aware event filter for FlowFees.FeeParametersChanged.Using SystemContractsForChain and AddressLocation ensures the fully-qualified event type matches the deployed address.
51-68: LGTM: wrapper goroutine cleanly forwards inner subscription and closes channel.Buffered outer channel and graceful shutdown look good.
79-85: FeeParamsEventSubscriber startHeight is sourced from persistedLatestCadenceHeightand validated viaGetBlockHeaderByHeightin StartEventIngestion, satisfying the “known-good height” requirement.
m-Peter
force-pushed
the
mpeter/track-surge-factor
branch
from
f8a2fbf to
a5efde3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
services/ingestion/engine.go (1)
193-208: Guard nil payloads and remove hard-coded height in errorPrevent nil deref on empty events; return a clear error without a bogus height; add a small info log on successful update.
-func (e *Engine) processFeeParamsEvents(events *models.FeeParamsEvents) error { - err := e.withBatch( - func(batch *pebbleDB.Batch) error { - return e.feeParameters.Store(events.FeeParameters, batch) - }, - ) - if err != nil { - return fmt.Errorf( - "failed to update fee parameters for height: %d, during events ingestion: %w", - 1, - err, - ) - } - - return nil -} +func (e *Engine) processFeeParamsEvents(ev *models.FeeParamsEvents) error { + if ev == nil || ev.FeeParameters == nil { + return nil + } + if err := e.withBatch(func(batch *pebbleDB.Batch) error { + return e.feeParameters.Store(ev.FeeParameters, batch) + }); err != nil { + return fmt.Errorf("failed to update fee parameters during events ingestion: %w", err) + } + e.log.Info().Msg("updated fee parameters") + return nil +}
🧹 Nitpick comments (3)
bootstrap/bootstrap.go (1)
659-667: Bootstrap default fee parameters only when missing; add log for operator visibilityLogic correctly stores defaults on ErrEntityNotFound and fails fast on other errors. Add an info log when bootstrapping to make initialization obvious in ops logs.
feeParameters := pebble.NewFeeParameters(store) if _, err = feeParameters.Get(); errors.Is(err, errs.ErrEntityNotFound) { - if err := feeParameters.Store(models.DefaultFeeParameters, batch); err != nil { + logger.Info().Msg("bootstrapping default fee parameters (not found)") + if err := feeParameters.Store(models.DefaultFeeParameters, batch); err != nil { return nil, nil, fmt.Errorf("failed to bootstrap fee parameters: %w", err) } } else if err != nil { return nil, nil, fmt.Errorf("failed to load latest fee parameters: %w", err) }services/ingestion/engine.go (2)
128-130: Rename local to avoid confusion with select case variableUse a distinct name for the fee-params channel to improve readability.
- feeParamsEvents := e.feeParamsSubscriber.Subscribe(ctx) + feeParamsCh := e.feeParamsSubscriber.Subscribe(ctx)Note: Update corresponding select case receive accordingly.
152-166: Handle fee-params channel with clearer naming; consider not stopping the engine on channel close
- Apply the variable rename from above in this block.
- Optional: instead of returning nil when the fee-params channel closes, log and continue processing block events (unless both channels are expected to be fatal when closed).
- case feeParams, ok := <-feeParamsEvents: + case feeParams, ok := <-feeParamsCh: if !ok { - return nil + e.log.Warn().Msg("fee-params subscription closed") + return nil // or continue, depending on desired resiliency }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
bootstrap/bootstrap.go(5 hunks)models/events.go(3 hunks)services/ingestion/engine.go(5 hunks)services/ingestion/engine_test.go(9 hunks)services/ingestion/event_subscriber.go(3 hunks)services/ingestion/fee_params_subscriber.go(1 hunks)services/ingestion/mocks/FeeParamsSubscriber.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- services/ingestion/fee_params_subscriber.go
- services/ingestion/engine_test.go
- models/events.go
- services/ingestion/event_subscriber.go
- services/ingestion/mocks/FeeParamsSubscriber.go
🧰 Additional context used
🧬 Code graph analysis (2)
services/ingestion/engine.go (5)
services/ingestion/event_subscriber.go (1)
EventSubscriber(27-33)services/ingestion/fee_params_subscriber.go (1)
FeeParamsSubscriber(23-25)storage/index.go (1)
FeeParametersIndexer(106-110)models/events.go (1)
FeeParamsEvents(268-271)storage/pebble/fee_parameters.go (1)
FeeParameters(14-17)
bootstrap/bootstrap.go (7)
storage/pebble/storage.go (1)
Storage(13-16)storage/index.go (5)
BlockIndexer(12-59)TransactionIndexer(84-95)ReceiptIndexer(61-82)TraceIndexer(97-104)FeeParametersIndexer(106-110)models/fee_parameters.go (2)
FeeParameters(21-25)DefaultFeeParameters(15-19)storage/pebble/fee_parameters.go (2)
FeeParameters(14-17)NewFeeParameters(19-23)services/ingestion/fee_params_subscriber.go (1)
NewFeeParamsEventSubscriber(36-49)services/ingestion/engine.go (1)
NewEventIngestionEngine(61-99)models/errors/errors.go (1)
ErrEntityNotFound(38-38)
⏰ 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 (7)
bootstrap/bootstrap.go (5)
54-60: LGTM: storage wiring includes FeeParameters indexerThe new field is correctly added to Storages. No issues.
185-191: LGTM: fee-params subscriber initializationSubscriber is instantiated with the correct dependencies and start height.
193-209: LGTM: engine wiring passes fee-params subscriber and indexerConstructor call matches the updated signature; ordering of arguments is correct.
676-683: LGTM: storages return includes FeeParameters indexerConstructor wiring is consistent.
320-331: Verify RPC handlers apply surge factor
I didn’t find any usage ofsurgeFactoror the injectedFeeParametersin theeth_gasPrice,eth_maxPriorityFeePerGas,eth_feeHistory, oreth_sendRawTransactionhandlers. Please confirm these RPCs multiply the base fee by the latest surge factor (with correct UFix64 scaling) and thateth_sendRawTransactionrejects gas prices below that threshold.services/ingestion/engine.go (2)
43-59: LGTM: Engine gains fee-params subscriber and indexerNew fields are appropriate and maintain symmetry with existing storages/subscribers.
61-99: LGTM: constructor signature and assignmentsAll new parameters are threaded through and assigned correctly; logging context preserved.
m-Peter
force-pushed
the
mpeter/track-surge-factor
branch
from
a5efde3 to
407106e
Compare
m-Peter
force-pushed
the
mpeter/track-surge-factor
branch
from
407106e to
994fcbf
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/ingestion/engine_test.go (1)
511-513: Fix compile error: cannot range over intGo cannot range over an int. Replace with a classic for-loop.
- for i := range eventCount { + for i := 0; i < eventCount; i++ {
♻️ Duplicate comments (1)
bootstrap/bootstrap.go (1)
651-658: LGTM: unexpected storage errors now propagated during fee-parameter bootstrapThis addresses the prior suggestion to fail fast on non-not-found errors. Nice.
🧹 Nitpick comments (8)
storage/index.go (2)
106-110: Document semantics and expected errors for FeeParametersIndexer.Get()Please add doc comments clarifying:
- Get returns the latest fee parameters (no historical lookup).
- Expected errors: models/errors.ErrEntityNotFound when none exist.
- Store: batch may be nil.
This prevents ambiguity for API/ingestion callers and aligns with bootstrap usage.
type FeeParametersIndexer interface { - Store(feeParameters *models.FeeParameters, batch *pebble.Batch) error - - Get() (*models.FeeParameters, error) + // Store persists fee parameters; batch may be nil. + Store(feeParameters *models.FeeParameters, batch *pebble.Batch) error + // Get returns the latest fee parameters. + // Returns errors.ErrEntityNotFound when no fee parameters have been stored. + Get() (*models.FeeParameters, error) }
106-110: Optional: consider GetLatest name for clarityIf feasible, renaming Get() → GetLatest() will read better alongside other indexers. Non-blocking given current scope.
services/ingestion/engine_test.go (1)
57-58: Use mock constructor to assert expectations and avoid silent passesInitialize the mock with the testing.T-aware constructor so unexpected calls fail fast and assertions run on cleanup.
- feeParams := &storageMock.FeeParametersIndexer{} + feeParams := storageMock.NewFeeParametersIndexer(t)(Apply at all occurrences above.)
Also applies to: 139-140, 271-273, 382-383, 469-470
bootstrap/bootstrap.go (2)
235-237: Typo in commentMinor: “Publick Key” → “Public Key”.
- // Skip account keys that do not use the same Publick Key as the + // Skip account keys that do not use the same Public Key as the
607-615: Rename evmBlokcHeight and fix printf arg for accuracySpelling nit and ensure the formatted height matches the variable (not a hard-coded 0).
- evmBlokcHeight := uint64(0) + evmBlockHeight := uint64(0) @@ - snapshot, err := registerStore.GetSnapshotAt(evmBlokcHeight) + snapshot, err := registerStore.GetSnapshotAt(evmBlockHeight) @@ - return nil, nil, fmt.Errorf("could not get register snapshot at block height %d: %w", 0, err) + return nil, nil, fmt.Errorf("could not get register snapshot at block height %d: %w", evmBlockHeight, err) @@ - err = registerStore.Store(delta.GetUpdates(), evmBlokcHeight, batch) + err = registerStore.Store(delta.GetUpdates(), evmBlockHeight, batch)Also applies to: 629-629
storage/mocks/FeeParametersIndexer.go (3)
17-45: Get() will panic if not stubbed — add a tiny helper to set safe defaults in tests.Current behavior is fine, but tests that forget to stub will panic. Consider a small convenience helper (outside the generated file) to pre-wire defaults.
Example helper (new file; keep generator output untouched):
// storage/mocks/fee_parameters_indexer_helpers_test.go package mocks import ( "github.com/onflow/flow-evm-gateway/models" "github.com/stretchr/testify/mock" ) func NewFeeParametersIndexerWithDefaults(t interface { mock.TestingT Cleanup(func()) }, params *models.FeeParameters, err error) *FeeParametersIndexer { m := NewFeeParametersIndexer(t) m.On("Get").Return(params, err) return m }
47-63: Store() expectations: prefer mock.Anything for batch and a default nil return.The pebble.Batch pointer equality can be brittle. In tests, set a permissive matcher and default nil error to avoid accidental panics.
Suggested stubs in tests:
idx := NewFeeParametersIndexer(t) idx.On("Store", mock.Anything, mock.Anything).Return(nil)
65-77: Add compile-time conformance assertion to catch interface drift.Do this in a separate test file to avoid editing generated code.
Add:
// storage/mocks/fee_parameters_indexer_contract_test.go package mocks import "github.com/onflow/flow-evm-gateway/storage" var _ storage.FeeParametersIndexer = (*FeeParametersIndexer)(nil)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
Makefile(1 hunks)api/api.go(7 hunks)bootstrap/bootstrap.go(5 hunks)models/events.go(5 hunks)models/fee_parameters.go(1 hunks)services/ingestion/engine.go(4 hunks)services/ingestion/engine_test.go(10 hunks)services/ingestion/event_subscriber.go(3 hunks)services/requester/requester.go(3 hunks)storage/index.go(1 hunks)storage/mocks/FeeParametersIndexer.go(1 hunks)storage/pebble/fee_parameters.go(1 hunks)storage/pebble/keys.go(1 hunks)tests/e2e_web3js_test.go(2 hunks)tests/web3js/eth_gas_price_surge_test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- storage/pebble/keys.go
- tests/e2e_web3js_test.go
- tests/web3js/eth_gas_price_surge_test.js
- services/ingestion/engine.go
- Makefile
- models/fee_parameters.go
- api/api.go
- models/events.go
- services/ingestion/event_subscriber.go
- services/requester/requester.go
🧰 Additional context used
🧬 Code graph analysis (5)
storage/index.go (2)
models/fee_parameters.go (1)
FeeParameters(21-25)storage/pebble/fee_parameters.go (1)
FeeParameters(14-17)
storage/pebble/fee_parameters.go (3)
storage/index.go (1)
FeeParametersIndexer(106-110)models/fee_parameters.go (2)
FeeParameters(21-25)NewFeeParametersFromBytes(42-49)storage/pebble/storage.go (1)
Storage(13-16)
storage/mocks/FeeParametersIndexer.go (3)
storage/index.go (1)
FeeParametersIndexer(106-110)models/fee_parameters.go (1)
FeeParameters(21-25)storage/pebble/fee_parameters.go (1)
FeeParameters(14-17)
services/ingestion/engine_test.go (2)
storage/index.go (1)
FeeParametersIndexer(106-110)storage/mocks/FeeParametersIndexer.go (1)
FeeParametersIndexer(13-15)
bootstrap/bootstrap.go (5)
storage/pebble/storage.go (1)
Storage(13-16)storage/index.go (1)
FeeParametersIndexer(106-110)models/fee_parameters.go (2)
FeeParameters(21-25)DefaultFeeParameters(15-19)storage/pebble/fee_parameters.go (2)
FeeParameters(14-17)NewFeeParameters(19-23)models/errors/errors.go (1)
ErrEntityNotFound(38-38)
⏰ 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 (2)
services/ingestion/engine_test.go (1)
480-490: Sanity‐check other ingestion loops sizing slices by eventCount
Grep shows loops in services/ingestion/event_subscriber.go (lines 348, 428) and services/ingestion/engine_test.go (line 511) that allocate slices by eventCount—confirm your fix covers these and assigns every index even if eventCount changes.storage/mocks/FeeParametersIndexer.go (1)
1-4: Auto-generated mock looks correct.Matches storage/index.go’s FeeParametersIndexer (Get, Store) and uses testify/mock idioms. No blocking issues.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/requester/remote_cadence_arch.go (1)
64-71: Data race on cachedCalls map (concurrent Run invocations).
rca.cachedCallsis read inRunand written inrunCallwithout synchronization. IfRemoteCadenceArchis used concurrently (likely during parallel dry-runs), this will race.Apply a mutex to guard the map:
type RemoteCadenceArch struct { blockHeight uint64 client *CrossSporkClient chainID flow.ChainID cachedCalls map[string]evmTypes.Data + cacheMux sync.RWMutex } func (rca *RemoteCadenceArch) Run(input []byte) ([]byte, error) { key := hex.EncodeToString(crypto.Keccak256(input)) - if result, ok := rca.cachedCalls[key]; ok { + rca.cacheMux.RLock() + result, ok := rca.cachedCalls[key] + rca.cacheMux.RUnlock() + if ok { return result, nil } evmResult, err := rca.runCall(input) if err != nil { return nil, err } return evmResult.ReturnedData, nil } func (rca *RemoteCadenceArch) runCall(input []byte) (*evmTypes.ResultSummary, error) { ... key := hex.EncodeToString(crypto.Keccak256(input)) - rca.cachedCalls[key] = evmResult.ReturnedData + rca.cacheMux.Lock() + rca.cachedCalls[key] = evmResult.ReturnedData + rca.cacheMux.Unlock() return evmResult, nil }Also applies to: 118-121
♻️ Duplicate comments (3)
services/requester/requester.go (2)
53-61: Document nil-safety for feeParams in the interface.Make it explicit that implementations must handle
nil(treat as surgeFactor=1x) to avoid panics and preserve default behavior when params are unavailable.type Requester interface { - // SendRawTransaction will submit signed transaction data to the network. - // The submitted EVM transaction hash is returned. + // SendRawTransaction will submit signed transaction data to the network. + // The submitted EVM transaction hash is returned. + // feeParams may be nil; implementations must handle nil defensively + // (equivalent to surgeFactor=1x) and must not panic. SendRawTransaction( ctx context.Context, data []byte, feeParams *models.FeeParameters, ) (common.Hash, error) }
235-237: Nil-deref risk and incorrect EIP-1559 enforcement (uses fee cap via GasPrice).
- Dereferencing
feeParamswithout a nil check will panic.- For dynamic-fee txs,
tx.GasPrice()returns the fee cap, not the effective price. Enforcing against it can admit underpriced txs.Apply this minimal fix (nil-safe + effective price computation). You’ll need a base fee value; wire in the latest EVM base fee (e.g., from the latest header/view) and pass it here.
- gasPrice := feeParams.CalculateGasPrice(e.config.GasPrice) - if tx.GasPrice().Cmp(gasPrice) < 0 && e.config.EnforceGasPrice { - return common.Hash{}, errs.NewTxGasPriceTooLowError(gasPrice) + // Determine minimum acceptable price (base * surge). + var minGasPrice *big.Int + if feeParams == nil { + minGasPrice = new(big.Int).Set(e.config.GasPrice) + } else { + minGasPrice = feeParams.CalculateGasPrice(e.config.GasPrice) + } + + // Compute the tx's effective offered price: + // - Legacy: GasPrice() + // - DynamicFee: min(feeCap, baseFee + tipCap) + var effective *big.Int + if tx.Type() == types.DynamicFeeTx { + baseFee, err := e.latestBaseFee() // TODO: implement: fetch from latest EVM header/view + if err != nil { + return common.Hash{}, fmt.Errorf("failed to load base fee: %w", err) + } + sum := new(big.Int).Add(tx.GasTipCap(), baseFee) + if sum.Cmp(tx.GasFeeCap()) < 0 { + effective = sum + } else { + effective = new(big.Int).Set(tx.GasFeeCap()) + } + } else { + effective = tx.GasPrice() + } + + if e.config.EnforceGasPrice && effective.Cmp(minGasPrice) < 0 { + return common.Hash{}, errs.NewTxGasPriceTooLowError(minGasPrice) }Optionally, factor out helper(s):
func (e *EVM) latestBaseFee() (*big.Int, error) { /* load from latest EVM header/view */ }To confirm the appropriate helper(s) in your go-ethereum version, run:
#!/bin/bash set -e version=$(grep -m1 "github.com/ethereum/go-ethereum" go.mod | awk '{print $2}') echo "geth version: $version" rm -rf geth_repo git clone --depth 1 --branch "$version" https://github.com/ethereum/go-ethereum.git geth_repo rg -nP "EffectiveGas.*|effectiveGasPrice|GasFeeCap|GasTipCap" geth_repo/core/types/*.go -n -C2bootstrap/bootstrap.go (1)
658-666: Avoid clobbering existing fee parameters; preserve non-surge fields and fail fast on unexpected read errors.Currently we always overwrite with defaults for non-surge fields, which can regress Inclusion/Execution costs on restart. Also, propagate storage read errors. Add a timeout to the network call to prevent hanging startup.
Apply:
- feeParameters := pebble.NewFeeParameters(store) - currentFeeParams, err := getNetworkFeeParams(context.Background(), config, client, logger) - if err != nil { - return nil, nil, fmt.Errorf("failed to fetch current fees surge factor: %w", err) - } - if err := feeParameters.Store(currentFeeParams, batch); err != nil { - return nil, nil, fmt.Errorf("failed to bootstrap fee parameters: %w", err) - } + feeParameters := pebble.NewFeeParameters(store) + // Read existing; fail fast on unexpected errors. + existingParams, getErr := feeParameters.Get() + if getErr != nil && !errors.Is(getErr, errs.ErrEntityNotFound) { + return nil, nil, fmt.Errorf("failed to load latest fee parameters: %w", getErr) + } + // Fetch network surge factor with a bounded timeout. + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + networkParams, err := getNetworkFeeParams(ctx, config, client, logger) + if err != nil { + return nil, nil, fmt.Errorf("failed to fetch current fees surge factor: %w", err) + } + if errors.Is(getErr, errs.ErrEntityNotFound) { + // Bootstrap fresh. + if err := feeParameters.Store(networkParams, batch); err != nil { + return nil, nil, fmt.Errorf("failed to bootstrap fee parameters: %w", err) + } + } else { + // Preserve existing non-surge fields; update surge only. + existingParams.SurgeFactor = networkParams.SurgeFactor + if err := feeParameters.Store(existingParams, batch); err != nil { + return nil, nil, fmt.Errorf("failed to update fee parameters: %w", err) + } + }
🧹 Nitpick comments (4)
services/requester/remote_cadence_arch.go (1)
103-111: Honor caller cancellation/timeouts for script execution.
ExecuteScriptAtBlockHeightis called withcontext.Background(), ignoring upstream cancellation. Consider threading a context with timeout from the caller or adding an internal timeout.services/requester/utils.go (1)
11-12: Doc comment grammar.Nit: make the exported comment read naturally.
-// ReplaceAddresses replace the addresses based on the network +// ReplaceAddresses replaces import addresses for known system contracts based on the network.bootstrap/bootstrap.go (2)
821-827: Tighten type assertion.Minor clarity/readability improvement.
- // sanity check, should never occur - if _, ok := val.(cadence.UFix64); !ok { - return nil, fmt.Errorf("failed to convert surgeFactor %v to UFix64, got type: %T", val, val) - } - - surgeFactor := val.(cadence.UFix64) + surgeFactor, ok := val.(cadence.UFix64) + if !ok { + return nil, fmt.Errorf("unexpected surgeFactor type %T (value: %v)", val, val) + }
828-831: Log human-friendly surge factor.Raw UFix64 logs as scaled uint; add normalized value for operators.
- logger.Debug(). - Uint64("surge-factor", uint64(surgeFactor)). - Msg("get current surge factor executed") + logger.Debug(). + Uint64("surge-factor-raw", uint64(surgeFactor)). + Float64("surge-factor", float64(uint64(surgeFactor))/1e8). + Msg("get current surge factor executed")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
bootstrap/bootstrap.go(7 hunks)bootstrap/cadence/get_fees_surge_factor.cdc(1 hunks)models/fee_parameters.go(1 hunks)services/requester/batch_tx_pool.go(2 hunks)services/requester/remote_cadence_arch.go(1 hunks)services/requester/requester.go(4 hunks)services/requester/single_tx_pool.go(1 hunks)services/requester/utils.go(1 hunks)storage/pebble/fee_parameters.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- models/fee_parameters.go
- storage/pebble/fee_parameters.go
🧰 Additional context used
🧬 Code graph analysis (5)
services/requester/single_tx_pool.go (1)
services/requester/utils.go (1)
ReplaceAddresses(12-32)
services/requester/remote_cadence_arch.go (1)
services/requester/utils.go (1)
ReplaceAddresses(12-32)
services/requester/batch_tx_pool.go (1)
services/requester/utils.go (1)
ReplaceAddresses(12-32)
services/requester/requester.go (3)
models/fee_parameters.go (1)
FeeParameters(23-27)models/errors/errors.go (1)
NewTxGasPriceTooLowError(61-66)services/requester/utils.go (1)
ReplaceAddresses(12-32)
bootstrap/bootstrap.go (7)
storage/pebble/storage.go (1)
Storage(13-16)storage/index.go (1)
FeeParametersIndexer(106-110)storage/pebble/fee_parameters.go (2)
FeeParameters(14-17)NewFeeParameters(19-23)models/fee_parameters.go (2)
FeeParameters(23-27)DefaultFeeParameters(15-21)config/config.go (1)
Config(33-114)services/requester/cross-spork_client.go (1)
CrossSporkClient(108-113)services/requester/utils.go (1)
ReplaceAddresses(12-32)
⏰ 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 (14)
bootstrap/cadence/get_fees_surge_factor.cdc (1)
1-5: Surge factor script looks correct and minimal.Directly returning
FlowFees.getFeeParameters().surgeFactoris fine, and address injection will be handled byReplaceAddresses(now includingFlowFees).services/requester/remote_cadence_arch.go (1)
106-106: Rename to ReplaceAddresses: LGTM.Updated call site matches the exported helper; behavior unchanged.
services/requester/single_tx_pool.go (1)
101-101: Rename to ReplaceAddresses: LGTM.Call site updated to exported helper; consistent with other requester components.
services/requester/batch_tx_pool.go (2)
221-221: Batch-run address replacement: LGTM.Updated to exported
ReplaceAddresses(batchRunTxScript, ...).
257-257: Single-run address replacement: LGTM.Updated to exported
ReplaceAddresses(runTxScript, ...).services/requester/utils.go (1)
15-20: Include FlowFees in replacements: LGTM.This is required for the new fee-parameters Cadence scripts. Good catch.
services/requester/requester.go (1)
451-455: Rename to ReplaceAddresses: LGTM.Latest-height script now correctly uses the exported address replacer.
bootstrap/bootstrap.go (7)
5-5: Embed import looks good.Necessary for go:embed usage.
13-13: Cadence import is correct.Required for UFix64 assertions and values.
55-58: Embedding surge-factor script is appropriate.Path/name are clear; no issues spotted.
61-68: Wiring FeeParameters into Storages is sane.Keeps persistence concerns localized and injectable.
202-203: Pass FeeParameters to ingestion engine.Correctly exposes storage to event processing.
326-327: Expose FeeParameters to API layer.Allows surge-adjusted pricing at the endpoints.
675-682: Storages return includes FeeParameters.End-to-end wiring complete.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
bootstrap/cadence/get_fees_surge_factor.cdc (3)
3-5: Prefer pub for widest Cadence compatibilityIf you intend to run this script against older sporks/Cadence versions, consider using
pub(synonym ofaccess(all)) to avoid parser/version surprises.-access(all) fun main(): UFix64 { +pub fun main(): UFix64 { return FlowFees.getFeeParameters().surgeFactor }
1-5: Add a brief doc comment for intentSmall clarity win for future readers.
+// Returns the current surge factor from FlowFees. +// Used by bootstrap to seed FeeParameters.SurgeFactor. import FlowFees
3-5: Optional: assert basic invariant (non-zero surge factor)If protocol guarantees surgeFactor > 0.0, a post-condition will fail fast on bad state. Validate the allowable range before adopting.
-pub fun main(): UFix64 { - return FlowFees.getFeeParameters().surgeFactor -} +pub fun main(): UFix64 { + post { result > 0.0: "surgeFactor must be > 0.0" } + return FlowFees.getFeeParameters().surgeFactor +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bootstrap/cadence/get_fees_surge_factor.cdc(1 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)
bootstrap/cadence/get_fees_surge_factor.cdc (1)
1-1: Confirm import resolver injects FlowFees address
bootstrap/cadence/get_fees_surge_factor.cdc:1usesimport FlowFeeswithout an explicit address. Verify thatCrossSporkClient.ExecuteScriptAtLatestBlock(configured insetupCrossSporkClient) injects the correctFlowFeesaddress fromsystemcontracts.SystemContractsForChain(chainID)for mainnet, testnet and all sporks.
m-Peter
force-pushed
the
mpeter/track-surge-factor
branch
from
f6dd3c0 to
351ba96
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
api/api.go (1)
832-846: FeeHistory: slice length mismatch on error; fallback instead of continueOn fee parameter retrieval error you log then continue, but baseFees was already appended → Reward/BaseFee/GasUsedRatio can diverge in length, violating the API contract. Fill rewards using base gas price and avoid index shadowing.
- blockRewards := make([]*hexutil.Big, len(rewardPercentiles)) - feeParams, err := b.feeParameters.Get() - if err != nil { - b.logger.Warn(). - Uint64("height", blockHeight). - Err(err). - Msg("failed to get fee parameters for block in fee history") - - continue - } - gasPrice := feeParams.CalculateGasPrice(b.config.GasPrice) - for i := range rewardPercentiles { - blockRewards[i] = (*hexutil.Big)(gasPrice) - } + blockRewards := make([]*hexutil.Big, len(rewardPercentiles)) + feeParams, err := b.feeParameters.Get() + var gasPrice *big.Int + if err != nil { + b.logger.Warn(). + Uint64("height", blockHeight). + Err(err). + Msg("fee parameters unavailable; using base gas price in fee history") + gasPrice = b.config.GasPrice + } else { + gasPrice = feeParams.CalculateGasPrice(b.config.GasPrice) + } + for j := range rewardPercentiles { + blockRewards[j] = (*hexutil.Big)(gasPrice) + }
🧹 Nitpick comments (10)
Makefile (1)
96-107: Reduce duplication in mock generation (loop over interfaces)Keeps the list in one place and makes future additions (like FeeParametersIndexer) a one‑liner change.
Apply within the generate target:
generate: go install github.com/vektra/mockery/[email protected] - mockery --dir=storage --name=BlockIndexer --output=storage/mocks - mockery --dir=storage --name=ReceiptIndexer --output=storage/mocks - mockery --dir=storage --name=TransactionIndexer --output=storage/mocks - mockery --dir=storage --name=TraceIndexer --output=storage/mocks - mockery --dir=storage --name=FeeParametersIndexer --output=storage/mocks + INDEXER_IFACES="BlockIndexer ReceiptIndexer TransactionIndexer TraceIndexer FeeParametersIndexer"; \ + for iface in $$INDEXER_IFACES; do \ + mockery --dir=storage --name=$$iface --output=storage/mocks; \ + done mockery --all --dir=services/traces --output=services/traces/mocks mockery --all --dir=services/ingestion --output=services/ingestion/mocks mockery --dir=models --name=Engine --output=models/mocksmodels/events.go (3)
37-44: Minor doc comment nitCollapse the two-line summary into one line for consistency with nearby helpers.
-// isFeeParametersChangedEvent checks whether the given event contains updates -// -// to Flow fees parameters. +// isFeeParametersChangedEvent checks whether the given event contains updates to Flow fees parameters.
55-56: Last-wins semantics for fee parameters — confirm intentOnly a single pointer is stored; multiple FeeParametersChanged events in one Flow block will result in the last one winning. If that’s intended, consider documenting it; otherwise, collect all and decide in ingestion.
Would you like a small patch to store a slice and pick the latest deterministically (by EventIndex)?
205-209: Accessor — consider clarifying “last update”Tiny doc tweak to make last-wins explicit.
-// FeeParameters returns any updates to the Flow fees parameters. +// FeeParameters returns the last Flow fees parameters update found in the block (if any).models/fee_parameters.go (2)
11-14: Use explicit uint64 precision to avoid implicit int conversionsMakes intent clearer and avoids relying on platform int size. Also build surgeFactorScale from uint64.
-const feeParamsPrecision = 100_000_000 - -var surgeFactorScale = big.NewInt(feeParamsPrecision) +const feeParamsPrecision uint64 = 100_000_000 + +var surgeFactorScale = new(big.Int).SetUint64(feeParamsPrecision)
33-42: Big-int math fix — LGTM; guard against accidental zero surge factorComputation is now safe. If SurgeFactor could ever be 0, RPC pricing and validation might drop to 0. If 0 is impossible by protocol, ignore; otherwise clamp to 1x as a safety net.
func (f *FeeParameters) CalculateGasPrice(currentGasPrice *big.Int) *big.Int { if currentGasPrice == nil { return new(big.Int) // zero } - // gasPrice = (currentGasPrice * surgeFactor) / feeParamsPrecision - surgeFactor := new(big.Int).SetUint64(uint64(f.SurgeFactor)) + // Treat 0 as 1x to avoid zeroing gas price if ever observed. + sf := uint64(f.SurgeFactor) + if sf == 0 { + sf = feeParamsPrecision + } + // gasPrice = (currentGasPrice * surgeFactor) / feeParamsPrecision + surgeFactor := new(big.Int).SetUint64(sf) gasPrice := new(big.Int).Mul(currentGasPrice, surgeFactor) return new(big.Int).Quo(gasPrice, surgeFactorScale) }I can also add a couple of unit tests to lock this behavior if helpful.
services/ingestion/engine.go (2)
51-52: Ensure feeParameters indexer is non-nilIf a caller passes nil, later Store will panic. Either enforce non-nil in the constructor or guard at callsite.
Would you like me to add a defensive check in the constructor that logs and returns an error when feeParameters is nil?
223-231: Nil-guard indexer to avoid panic and add a clear skip pathIf wiring ever passes a nil indexer (tests, mocks), this prevents a crash.
- if events.FeeParameters() != nil { - if err := e.feeParameters.Store(events.FeeParameters(), batch); err != nil { + if events.FeeParameters() != nil { + if e.feeParameters == nil { + e.log.Warn().Msg("fee parameters indexer is nil; skipping update") + } else if err := e.feeParameters.Store(events.FeeParameters(), batch); err != nil { return fmt.Errorf( "failed to update fee parameters for height: %d, during events ingestion: %w", events.CadenceHeight(), err, ) } }api/api.go (2)
185-191: Normalize error mapping in SendRawTransactionReturn JSON-RPC-shaped errors and emit metrics consistently via handleError.
- feeParams, err := b.feeParameters.Get() - if err != nil { - return common.Hash{}, err - } + feeParams, err := b.feeParameters.Get() + if err != nil { + return handleError[common.Hash](err, l, b.collector) + }
832-846: Optional: fetch fee params once to keep a single “latest” view per responseReduce repeated Get() calls and keep the response internally consistent for the request.
Example (outside the loop):
feeParams, err := b.feeParameters.Get() var gasPrice *big.Int if err != nil { b.logger.Warn().Err(err).Msg("fee parameters unavailable; using base gas price in fee history") gasPrice = b.config.GasPrice } else { gasPrice = feeParams.CalculateGasPrice(b.config.GasPrice) } // then use gasPrice for all blocks in this response
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
Makefile(1 hunks)api/api.go(7 hunks)bootstrap/bootstrap.go(7 hunks)bootstrap/cadence/get_fees_surge_factor.cdc(1 hunks)models/events.go(5 hunks)models/fee_parameters.go(1 hunks)services/ingestion/engine.go(4 hunks)services/ingestion/engine_test.go(10 hunks)services/ingestion/event_subscriber.go(3 hunks)services/requester/batch_tx_pool.go(2 hunks)services/requester/remote_cadence_arch.go(1 hunks)services/requester/requester.go(4 hunks)services/requester/single_tx_pool.go(1 hunks)services/requester/utils.go(1 hunks)storage/index.go(1 hunks)storage/mocks/FeeParametersIndexer.go(1 hunks)storage/pebble/fee_parameters.go(1 hunks)storage/pebble/keys.go(1 hunks)tests/e2e_web3js_test.go(2 hunks)tests/web3js/eth_gas_price_surge_test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- services/requester/batch_tx_pool.go
- storage/pebble/fee_parameters.go
- services/requester/utils.go
- services/ingestion/engine_test.go
- storage/pebble/keys.go
- bootstrap/cadence/get_fees_surge_factor.cdc
- storage/index.go
- services/requester/remote_cadence_arch.go
- storage/mocks/FeeParametersIndexer.go
- tests/web3js/eth_gas_price_surge_test.js
- services/requester/requester.go
- bootstrap/bootstrap.go
- services/requester/single_tx_pool.go
- tests/e2e_web3js_test.go
- services/ingestion/event_subscriber.go
🧰 Additional context used
🧬 Code graph analysis (3)
models/fee_parameters.go (1)
storage/pebble/fee_parameters.go (1)
FeeParameters(14-17)
api/api.go (1)
storage/index.go (1)
FeeParametersIndexer(106-110)
services/ingestion/engine.go (4)
storage/index.go (1)
FeeParametersIndexer(106-110)storage/mocks/FeeParametersIndexer.go (1)
FeeParametersIndexer(13-15)storage/pebble/fee_parameters.go (1)
FeeParameters(14-17)models/fee_parameters.go (1)
FeeParameters(23-27)
🔇 Additional comments (13)
Makefile (1)
103-103: FeeParametersIndexer mock generation — verifiedNo lingering AccountIndexer references found; generated mock present at storage/mocks/FeeParametersIndexer.go.
models/events.go (2)
16-19: Event type constant — LGTMUsing a single exported constant to match the Cadence qualified identifier is good.
139-146: Fail-fast on decode — LGTMEarly return on decode error is correct and prevents partial state.
models/fee_parameters.go (5)
15-21: Default 1.0 values — LGTMUsing 1.0 (scaled) as sane defaults reads well.
23-27: Struct definition — LGTMField names and Cadence tags align with the event payload.
29-31: RLP encode — LGTMStraightforward and consistent with other models.
44-51: RLP decode — LGTMError wrapping is useful; keeps payload context.
53-64: Cadence decode — LGTMTags match and error message provides event context.
services/ingestion/engine.go (1)
60-75: Constructor signature changed — call sites updated; tests not verifiedAll usages of NewEventIngestionEngine were updated to pass the new replayerConfig parameter (services/ingestion/engine_test.go — multiple tests; bootstrap/bootstrap.go). Run the unit test suite / CI to confirm no regressions.
api/api.go (4)
832-846: Summary says “per-block dynamic,” code uses “latest”; confirm intentAI summary claims per-block fee params; the code uses latest via Get() each iteration. The PR objectives specify latest surgeFactor, which matches the code. Please confirm.
1027-1034: GasPrice fallback behavior: LGTMWarn-and-fallback to base gas price matches spec and improves robustness.
1074-1081: MaxPriorityFeePerGas fallback behavior: LGTMConsistent with GasPrice and spec.
88-92: Surge-factor plumbing: wiring verified — NewBlockChainAPI call sites updated
Constructor field, parameter and assignment are consistent; bootstrap/bootstrap.go passes b.storages.FeeParameters in the correct position (bootstrap/bootstrap.go:319–329).
m-Peter
force-pushed
the
mpeter/track-surge-factor
branch
from
351ba96 to
405470c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
models/events.go (1)
139-146: Clarify multiple-events semantics or short‑circuit on first match.If a Flow block ever emits multiple FeeParametersChanged events, the last one wins. If “first match” is desired, break after assignment.
Apply if you want first-match and to save a few cycles:
if isFeeParametersChangedEvent(val) { feeParameters, err := decodeFeeParametersChangedEvent(val) if err != nil { return nil, err } e.feeParameters = feeParameters + break }services/ingestion/event_subscriber.go (1)
555-577: Compose filters via system contracts — good. Consider deduping event ID construction.blocksEventFilter is clear. The EVM event IDs are built in multiple places (see backfillSporkFromHeight at Lines 295-308). Factor those through evmEventFilter to avoid drift.
bootstrap/bootstrap.go (2)
658-666: Bootstrapping from live network only: consider fallback/skip-write behavior.If the script call fails, service startup fails. Optionally:
- Fallback to last persisted value on fetch error, only failing if both fetch and read fail.
- Skip Store when the fetched value equals the persisted one to avoid needless writes.
I can draft a small wrapper that reads existing params and conditionally stores on change.
805-836: Log surge factor as a string, not a raw UFix64 integer.Uint64 casts drop fixed-point scale; log the canonical string.
Apply:
- logger.Debug(). - Uint64("surge-factor", uint64(surgeFactor)). - Msg("get current surge factor executed") + logger.Debug(). + Str("surge-factor", surgeFactor.String()). + Msg("retrieved current surge factor")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
Makefile(1 hunks)api/api.go(7 hunks)bootstrap/bootstrap.go(7 hunks)bootstrap/cadence/get_fees_surge_factor.cdc(1 hunks)models/events.go(5 hunks)models/fee_parameters.go(1 hunks)services/ingestion/engine.go(4 hunks)services/ingestion/engine_test.go(10 hunks)services/ingestion/event_subscriber.go(3 hunks)services/requester/batch_tx_pool.go(2 hunks)services/requester/remote_cadence_arch.go(1 hunks)services/requester/requester.go(4 hunks)services/requester/single_tx_pool.go(1 hunks)services/requester/utils.go(1 hunks)storage/index.go(1 hunks)storage/mocks/FeeParametersIndexer.go(1 hunks)storage/pebble/fee_parameters.go(1 hunks)storage/pebble/keys.go(1 hunks)tests/e2e_web3js_test.go(2 hunks)tests/web3js/eth_gas_price_surge_test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (17)
- bootstrap/cadence/get_fees_surge_factor.cdc
- services/requester/remote_cadence_arch.go
- Makefile
- storage/pebble/keys.go
- services/requester/utils.go
- storage/index.go
- services/requester/batch_tx_pool.go
- services/requester/single_tx_pool.go
- tests/web3js/eth_gas_price_surge_test.js
- services/ingestion/engine.go
- storage/mocks/FeeParametersIndexer.go
- api/api.go
- models/fee_parameters.go
- tests/e2e_web3js_test.go
- storage/pebble/fee_parameters.go
- services/ingestion/engine_test.go
- services/requester/requester.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-18T19:26:37.579Z
Learnt from: peterargue
PR: onflow/flow-evm-gateway#617
File: api/stream.go:62-67
Timestamp: 2024-10-18T19:26:37.579Z
Learning: In the `flow-evm-gateway` project, within the Go file `api/stream.go`, the `prepareBlockResponse` method includes the Bloom filter as the field `LogsBloom` in the returned `Block` struct.
Applied to files:
services/ingestion/event_subscriber.go
🧬 Code graph analysis (3)
bootstrap/bootstrap.go (6)
storage/index.go (1)
FeeParametersIndexer(106-110)storage/pebble/fee_parameters.go (2)
FeeParameters(14-17)NewFeeParameters(19-23)models/fee_parameters.go (2)
FeeParameters(23-27)DefaultFeeParameters(15-21)config/config.go (1)
Config(33-114)services/requester/cross-spork_client.go (1)
CrossSporkClient(108-113)services/requester/utils.go (1)
ReplaceAddresses(12-32)
services/ingestion/event_subscriber.go (1)
models/events.go (1)
FeeParametersChangedQualifiedIdentifier(18-18)
models/events.go (2)
storage/pebble/fee_parameters.go (1)
FeeParameters(14-17)models/fee_parameters.go (1)
FeeParameters(23-27)
⏰ 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 (11)
models/events.go (4)
16-19: Event identifier addition looks good.Constant name and usage are consistent with the existing EVM identifiers.
37-44: Type guard mirrors existing checks.Nil-safe and consistent with EVM event checks.
55-55: State carrier for fee parameters is appropriate.Private field with a public accessor keeps surface minimal.
205-208: Accessor reads well.Straightforward getter; no concurrency concerns in this context.
services/ingestion/event_subscriber.go (3)
149-151: Subscribing with blocksEventFilter ensures FeeParametersChanged events are streamed.Nice upgrade; pairs well with decode path in models/events.go.
474-496: Recovering only EVM events is correct here.Limiting fetchMissingData to EVM types avoids unrelated FeeParameters noise during transaction recovery.
583-605: evmEventFilter centralization LGTM.Single source of truth for EVM event IDs reduces copy/paste.
bootstrap/bootstrap.go (4)
55-58: Embedding surge-factor script wired correctly.Path is relative to this package; ReplaceAddresses will fix imports per chain.
60-68: Plumbing FeeParameters into Storages is appropriate.Indexer exposure aligns with later API/engine usage.
202-203: Engine receives FeeParameters indexer.This unblocks persistent updates on FeeParametersChanged.
326-327: API receives FeeParameters indexer.Needed for surge-aware fee responses and validation.
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)
api/api.go (1)
88-92: Add nil-guard for feeParameters indexer.Prevent panics if wiring breaks; fail fast in the constructor.
func NewBlockChainAPI( @@ feeParameters storage.FeeParametersIndexer, rateLimiter RateLimiter, collector metrics.Collector, indexingResumedHeight uint64, ) *BlockChainAPI { + if feeParameters == nil { + logger.Panic().Msg("feeParameters indexer is required") + } return &BlockChainAPI{
♻️ Duplicate comments (3)
api/api.go (3)
843-846: Log skipped blocks in FeeHistory for observability.Silently continuing hides data gaps; warn with height and error.
- block, err := b.blocks.GetByHeight(blockHeight) - if err != nil { - continue - } + block, err := b.blocks.GetByHeight(blockHeight) + if err != nil { + b.logger.Warn(). + Uint64("height", blockHeight). + Err(err). + Msg("failed to load block for fee history; skipping") + continue + }
1034-1040: GasPrice fallback — LGTM and aligns with prior guidance.
1081-1087: MaxPriorityFeePerGas fallback — LGTM and consistent with GasPrice.
🧹 Nitpick comments (4)
api/api.go (2)
176-180: Avoid logging full raw transaction bytes.Log a short prefix to reduce log volume and accidental leakage of full signed payloads.
- l := b.logger.With(). - Str("endpoint", EthSendRawTransaction). - Str("input", input.String()). - Logger() + inHex := input.String() + if len(inHex) > 18 { + inHex = inHex[:18] + "…" + } + l := b.logger.With(). + Str("endpoint", EthSendRawTransaction). + Str("input_prefix", inHex). + Logger()
854-855: Avoid aliasing the same rewards slice across blocks.Append a copy so future modifications can’t retroactively alter prior entries.
- rewards = append(rewards, blockRewards) + brCopy := make([]*hexutil.Big, len(blockRewards)) + copy(brCopy, blockRewards) + rewards = append(rewards, brCopy)bootstrap/bootstrap.go (2)
658-666: Bound bootstrap network call; avoid hanging startup on AN issues.Use a timeout when fetching initial fee params.
- currentFeeParams, err := getNetworkFeeParams(context.Background(), config, client, logger) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + currentFeeParams, err := getNetworkFeeParams(ctx, config, client, logger) if err != nil { return nil, nil, fmt.Errorf("failed to fetch current fees surge factor: %w", err) }Optional: If availability is preferred over strictness, consider falling back to models.DefaultFeeParameters() with a WARN and allowing startup, while keeping ingestion responsible for updating.
812-835: getNetworkFeeParams: add timeout and improve logging.
- Bound ExecuteScript call with a short timeout.
- Log a human‑readable surge factor value.
func getNetworkFeeParams( ctx context.Context, config config.Config, client *requester.CrossSporkClient, logger zerolog.Logger, ) (*models.FeeParameters, error) { - val, err := client.ExecuteScriptAtLatestBlock( - ctx, + // Bound the network call; caller may already supply a timeout. + cctx, cancel := context.WithTimeout(ctx, 5*time.Second) + defer cancel() + val, err := client.ExecuteScriptAtLatestBlock( + cctx, requester.ReplaceAddresses(getFeesSurgeFactor, config.FlowNetworkID), nil, ) @@ - logger.Debug(). - Uint64("surge-factor", uint64(surgeFactor)). - Msg("get current surge factor executed") + logger.Debug(). + Str("surge-factor", surgeFactor.String()). + Msg("fetched current surge factor")Optional: validate surgeFactor > 0 to guard against bad on-chain data and fall back to defaults with a WARN.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
api/api.go(7 hunks)bootstrap/bootstrap.go(7 hunks)tests/web3js/eth_gas_price_surge_test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/web3js/eth_gas_price_surge_test.js
🧰 Additional context used
🧬 Code graph analysis (2)
bootstrap/bootstrap.go (6)
storage/pebble/storage.go (1)
Storage(13-16)storage/index.go (1)
FeeParametersIndexer(106-110)storage/pebble/fee_parameters.go (2)
FeeParameters(14-17)NewFeeParameters(19-23)models/fee_parameters.go (2)
FeeParameters(23-27)DefaultFeeParameters(15-21)services/requester/cross-spork_client.go (1)
CrossSporkClient(108-113)services/requester/utils.go (1)
ReplaceAddresses(12-32)
api/api.go (1)
storage/index.go (1)
FeeParametersIndexer(106-110)
⏰ 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 (6)
api/api.go (1)
825-836: FeeHistory: graceful fallback to base gas price — LGTM.This matches the agreed behavior when fee parameters are unavailable.
bootstrap/bootstrap.go (5)
55-59: Embed Cadence script — LGTM.
61-68: Storages wiring extended with FeeParameters — LGTM.Keeps API/ingestion plumbing cohesive.
192-203: Pass FeeParameters to ingestion engine — LGTM.
319-330: Pass FeeParameters to API — LGTM.
675-682: Return FeeParameters in Storages — LGTM.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
api/api.go (4)
185-191: Consider consistency with other pricing endpoints for SendRawTransaction.While falling back to default fee parameters is reasonable, this differs from the GasPrice/MaxPriorityFeePerGas endpoints which return the base price directly without applying surge factor calculations on fallback. Additionally, consider using
handleErrorfor consistent error handling and metrics.Consider this approach for consistency:
feeParams, err := b.feeParameters.Get() if err != nil { b.logger.Warn().Err(err).Msg("fee parameters unavailable; falling back to base gas price") - feeParams = models.DefaultFeeParameters() + // Use base parameters without surge factor like other endpoints + return handleError[common.Hash](b.evm.SendRawTransaction(ctx, input, nil), l, b.collector) } id, err := b.evm.SendRawTransaction(ctx, input, feeParams)
826-837: Optimize fee parameter retrieval in FeeHistory.Good fallback handling! However, retrieving fee parameters once before the loop is more efficient than the previous per-block retrieval mentioned in past reviews. The current implementation correctly maintains consistency across all returned blocks.
1035-1042: LGTM! Proper surge-adjusted gas price with graceful fallback.The implementation correctly retrieves fee parameters, calculates the surge-adjusted gas price, and falls back to the base price with appropriate logging when parameters are unavailable.
1082-1089: LGTM! Consistent implementation with GasPrice endpoint.The MaxPriorityFeePerGas implementation mirrors the GasPrice endpoint's behavior, maintaining consistency across pricing endpoints.
🧹 Nitpick comments (1)
api/api.go (1)
1035-1042: Consider extracting common gas price calculation logic.Both
GasPriceandMaxPriorityFeePerGascontain identical implementations. Consider extracting this into a helper method to follow DRY principles.+// calculateCurrentGasPrice returns the surge-adjusted gas price or falls back to base price +func (b *BlockChainAPI) calculateCurrentGasPrice() *big.Int { + feeParams, err := b.feeParameters.Get() + if err != nil { + b.logger.Warn().Err(err).Msg("fee parameters unavailable; falling back to base gas price") + return b.config.GasPrice + } + return feeParams.CalculateGasPrice(b.config.GasPrice) +} // GasPrice returns a suggestion for a gas price for legacy transactions. func (b *BlockChainAPI) GasPrice(ctx context.Context) (*hexutil.Big, error) { - feeParams, err := b.feeParameters.Get() - if err != nil { - b.logger.Warn().Err(err).Msg("fee parameters unavailable; falling back to base gas price") - return (*hexutil.Big)(b.config.GasPrice), nil - } - gasPrice := feeParams.CalculateGasPrice(b.config.GasPrice) - return (*hexutil.Big)(gasPrice), nil + return (*hexutil.Big)(b.calculateCurrentGasPrice()), nil } // MaxPriorityFeePerGas returns a suggestion for a gas tip cap for dynamic fee transactions. func (b *BlockChainAPI) MaxPriorityFeePerGas(ctx context.Context) (*hexutil.Big, error) { - feeParams, err := b.feeParameters.Get() - if err != nil { - b.logger.Warn().Err(err).Msg("fee parameters unavailable; falling back to base gas price") - return (*hexutil.Big)(b.config.GasPrice), nil - } - gasPrice := feeParams.CalculateGasPrice(b.config.GasPrice) - return (*hexutil.Big)(gasPrice), nil + return (*hexutil.Big)(b.calculateCurrentGasPrice()), nil }Also applies to: 1082-1089
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/api.go(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
api/api.go (2)
storage/index.go (1)
FeeParametersIndexer(106-110)models/fee_parameters.go (1)
DefaultFeeParameters(15-21)
⏰ 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 (2)
api/api.go (2)
88-88: LGTM! Proper integration of FeeParametersIndexer.The addition of the
feeParametersfield toBlockChainAPIand its initialization in the constructor follows the established pattern for dependency injection in this codebase.Also applies to: 101-101, 113-113
185-191: Surge-adjusted gas-price validation present — no action needed. EVM.SendRawTransaction computes gasPrice := feeParams.CalculateGasPrice(e.config.GasPrice) and rejects with NewTxGasPriceTooLowError when tx.GasPrice() < gasPrice and EnforceGasPrice is enabled (services/requester/requester.go — SendRawTransaction; models/fee_parameters.go — CalculateGasPrice).
m-Peter
force-pushed
the
mpeter/track-surge-factor
branch
from
7339c7a to
b89311e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
storage/pebble/fee_parameters.go (1)
25-39: Add nil guard for feeParameters parameter.Line 29 dereferences
feeParametersby callingToBytes()without checking for nil, which would panic. Past review comments flagged this issue.Apply this diff:
func (f *FeeParameters) Store(feeParameters *models.FeeParameters, batch *pebble.Batch) error { f.mu.Lock() defer f.mu.Unlock() + if feeParameters == nil { + return fmt.Errorf("nil fee parameters") + } + val, err := feeParameters.ToBytes() if err != nil { return err }services/requester/requester.go (2)
56-60: Document nil-safety contract for feeParams parameter.The interface adds
feeParams *models.FeeParametersbut doesn't document whethernilis permitted. Callers need to know:
- Can
feeParamsbenil?- How should implementations handle
nildefensively?Add documentation to the interface:
// SendRawTransaction will submit signed transaction data to the network. // The submitted EVM transaction hash is returned. + // feeParams may be nil; implementations must handle nil defensively by + // falling back to default fee parameters. SendRawTransaction( ctx context.Context, data []byte, feeParams *models.FeeParameters, ) (common.Hash, error)
236-239: Critical: Nil dereference and incorrect EIP-1559 gas price validation.Two major issues:
Nil dereference risk: Line 236 calls
feeParams.CalculateGasPrice()without checking for nil, which will panic if fee parameters are unavailable.Incorrect EIP-1559 handling: Line 237 uses
tx.GasPrice()which returns theGasFeeCapfor dynamic fee transactions, not the actual effective gas price paid. This allows EIP-1559 transactions to bypass minimum price enforcement even when their effective price is too low.Apply this diff to address both issues:
- gasPrice := feeParams.CalculateGasPrice(e.config.GasPrice) - if tx.GasPrice().Cmp(gasPrice) < 0 && e.config.EnforceGasPrice { - return common.Hash{}, errs.NewTxGasPriceTooLowError(gasPrice) + // Determine the minimum acceptable gas price considering the current fee parameters. + var minGasPrice *big.Int + if feeParams == nil { + // Fall back to base price (surge factor = 1x) if fee parameters are unavailable. + minGasPrice = new(big.Int).Set(e.config.GasPrice) + } else { + minGasPrice = feeParams.CalculateGasPrice(e.config.GasPrice) + } + + // Compute the transaction's actual gas price: + // - For legacy txs, GasPrice() is correct. + // - For EIP-1559 (DynamicFee) txs, GasPrice() returns fee cap, not effective price. + var txEffectivePrice *big.Int + if tx.Type() == types.DynamicFeeTxType { + // Use effective gas price calculation for EIP-1559 transactions + // Note: You'll need access to current base fee; consider fetching from latest block + baseFee := big.NewInt(1) // Placeholder - fetch actual base fee + txEffectivePrice = math.BigMin( + new(big.Int).Add(tx.GasTipCap(), baseFee), + tx.GasFeeCap(), + ) + } else { + txEffectivePrice = tx.GasPrice() + } + + if e.config.EnforceGasPrice && txEffectivePrice.Cmp(minGasPrice) < 0 { + return common.Hash{}, errs.NewTxGasPriceTooLowError(minGasPrice) }Note: For EIP-1559 validation, you'll need to obtain the current base fee from the latest block to compute the effective gas price accurately.
🧹 Nitpick comments (2)
storage/pebble/fee_parameters.go (1)
50-53: Wrap deserialization error for consistency.Line 52 returns the error from
NewFeeParametersFromByteswithout context, while line 47 wraps the storage error. For consistency and better debugging, wrap this error too.feeParameters, err := models.NewFeeParametersFromBytes(data) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to decode fee parameters: %w", err) }bootstrap/bootstrap.go (1)
805-835: LGTM with minor comment adjustment suggestion.The
getNetworkFeeParamsfunction correctly fetches the current surge factor from the network using the embedded Cadence script and handles errors appropriately.Consider revising the comment on line 821:
- // sanity check, should never occur + // Defensive check: script must return UFix64 surgeFactor, ok := val.(cadence.UFix64)The type assertion can legitimately fail if the Cadence script is modified to return a different type, so "should never occur" understates the defensive programming value here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
Makefile(1 hunks)api/api.go(7 hunks)bootstrap/bootstrap.go(7 hunks)bootstrap/cadence/get_fees_surge_factor.cdc(1 hunks)models/events.go(5 hunks)models/fee_parameters.go(1 hunks)services/ingestion/engine.go(4 hunks)services/ingestion/engine_test.go(10 hunks)services/ingestion/event_subscriber.go(3 hunks)services/requester/batch_tx_pool.go(2 hunks)services/requester/remote_cadence_arch.go(1 hunks)services/requester/requester.go(4 hunks)services/requester/single_tx_pool.go(1 hunks)services/requester/utils.go(1 hunks)storage/index.go(1 hunks)storage/mocks/FeeParametersIndexer.go(1 hunks)storage/pebble/fee_parameters.go(1 hunks)storage/pebble/keys.go(1 hunks)tests/e2e_web3js_test.go(2 hunks)tests/web3js/eth_gas_price_surge_test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- services/requester/remote_cadence_arch.go
- storage/pebble/keys.go
- services/ingestion/engine.go
- models/fee_parameters.go
- storage/index.go
- services/requester/single_tx_pool.go
- services/requester/utils.go
- Makefile
🧰 Additional context used
🧬 Code graph analysis (10)
api/api.go (2)
storage/index.go (1)
FeeParametersIndexer(106-110)models/fee_parameters.go (1)
DefaultFeeParameters(15-21)
bootstrap/bootstrap.go (6)
storage/pebble/storage.go (1)
Storage(13-16)storage/index.go (1)
FeeParametersIndexer(106-110)storage/pebble/fee_parameters.go (2)
FeeParameters(14-17)NewFeeParameters(19-23)models/fee_parameters.go (2)
FeeParameters(23-27)DefaultFeeParameters(15-21)config/config.go (1)
Config(43-124)services/requester/utils.go (1)
ReplaceAddresses(12-32)
services/requester/batch_tx_pool.go (1)
services/requester/utils.go (1)
ReplaceAddresses(12-32)
tests/web3js/eth_gas_price_surge_test.js (1)
tests/e2e-network/e2e_test.js (1)
assert(3-3)
services/ingestion/event_subscriber.go (1)
models/events.go (1)
FeeParametersChangedQualifiedIdentifier(18-18)
storage/pebble/fee_parameters.go (3)
storage/index.go (1)
FeeParametersIndexer(106-110)models/fee_parameters.go (2)
FeeParameters(23-27)NewFeeParametersFromBytes(44-51)storage/pebble/storage.go (1)
Storage(13-16)
storage/mocks/FeeParametersIndexer.go (3)
storage/index.go (1)
FeeParametersIndexer(106-110)storage/pebble/fee_parameters.go (1)
FeeParameters(14-17)models/fee_parameters.go (1)
FeeParameters(23-27)
services/ingestion/engine_test.go (2)
storage/index.go (1)
FeeParametersIndexer(106-110)storage/mocks/FeeParametersIndexer.go (1)
FeeParametersIndexer(13-15)
models/events.go (2)
storage/pebble/fee_parameters.go (1)
FeeParameters(14-17)models/fee_parameters.go (1)
FeeParameters(23-27)
services/requester/requester.go (3)
models/fee_parameters.go (1)
FeeParameters(23-27)models/errors/errors.go (1)
NewTxGasPriceTooLowError(61-66)services/requester/utils.go (1)
ReplaceAddresses(12-32)
⏰ 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 (36)
models/events.go (5)
16-18: LGTM – Event identifier added correctly.The new constant
FeeParametersChangedQualifiedIdentifierfollows the existing pattern and is correctly scoped.
37-44: LGTM – Consistent event detection pattern.The
isFeeParametersChangedEventfunction follows the same pattern asisBlockExecutedEventandisTransactionExecutedEvent, with proper nil checks.
55-55: LGTM – Field addition aligns with event decoding.The new
feeParametersfield properly extendsCadenceEventsto hold decoded fee parameter updates.
139-146: LGTM – Error handling improved.The decoding logic now correctly returns immediately on error instead of assigning invalid data. This addresses the past review comment and prevents overwriting errors when multiple matching events are encountered.
205-208: LGTM – Accessor follows existing conventions.The
FeeParameters()accessor is consistent with other accessors likeBlock(),Transactions(), andReceipts().services/requester/batch_tx_pool.go (2)
221-221: LGTM – Function rename applied correctly.The call site now uses the exported
ReplaceAddressesfunction consistently with the refactoring inservices/requester/utils.go.
257-257: LGTM – Function rename applied correctly.The call site now uses the exported
ReplaceAddressesfunction consistently with the refactoring inservices/requester/utils.go.tests/e2e_web3js_test.go (2)
6-6: LGTM – Import added for string formatting.The
fmtimport is correctly added to supportfmt.Sprintfin the new test.
127-163: LGTM – Surge factor test implementation is sound.The test correctly iterates over multiple surge factor values, sends Cadence transactions to update FlowFees parameters, and verifies no errors. This provides good coverage for the surge-factor update mechanism.
services/ingestion/engine_test.go (5)
57-57: LGTM – Test updated for new engine signature.The mock
FeeParametersIndexeris correctly instantiated and passed toNewEventIngestionEngine, aligning with the signature change.Also applies to: 77-77
139-139: LGTM – Test updated for new engine signature.The mock
FeeParametersIndexeris correctly instantiated and passed toNewEventIngestionEngine, aligning with the signature change.Also applies to: 158-158
271-271: LGTM – Test updated for new engine signature.The mock
FeeParametersIndexeris correctly instantiated and passed toNewEventIngestionEngine, aligning with the signature change.Also applies to: 282-282
382-382: LGTM – Test updated for new engine signature.The mock
FeeParametersIndexeris correctly instantiated and passed toNewEventIngestionEngine, aligning with the signature change.Also applies to: 393-393
469-469: LGTM – Test updated for new engine signature.The mock
FeeParametersIndexeris correctly instantiated and passed toNewEventIngestionEngine, aligning with the signature change.Also applies to: 489-489
bootstrap/cadence/get_fees_surge_factor.cdc (1)
1-5: LGTM – Script correctly retrieves surge factor.The Cadence script is minimal and correct: it imports
FlowFees, callsgetFeeParameters(), and returns thesurgeFactorfield. Theaccess(all)modifier is appropriate for a public query script.tests/web3js/eth_gas_price_surge_test.js (5)
7-11: LGTM! Gas price surge factor validation is correct.The test correctly validates that
eth_gasPricereturns the base gas price multiplied by the surge factor (100).
13-24: LGTM! Type conversion correctly uses BigInt.Line 21 properly converts the hex response to
BigIntbefore comparison with the expected value, addressing the previous review concern about Number/BigInt mismatch.
26-51: LGTM! Transaction rejection with low gas price is properly validated.The test correctly verifies that transactions submitted with a gas price below the surge-adjusted minimum are rejected with an appropriate error message.
53-82: LGTM! Comprehensive validation of surge-adjusted transaction acceptance.This test thoroughly validates transaction acceptance with the updated gas price, including block composition, effective gas price, and coinbase fee calculations.
84-106: LGTM! Fee history surge pricing validation is thorough.The test correctly validates that
eth_feeHistoryreturns surge-adjusted gas prices (0x3a98 = 100 × base price) for historical blocks.services/ingestion/event_subscriber.go (3)
149-149: LGTM! Subscription filter updated to include fee parameter events.Line 149 correctly uses
blocksEventFilterwhich now includes both EVM events and theFeeParametersChangedevent.
564-585: LGTM! Clean separation of EVM and fee parameter events.The
blocksEventFilterfunction correctly composes the baseevmEventFilterwith theFeeParametersChangedevent from the FlowFees contract. The address derivation using system contracts is appropriate.
587-614: LGTM! EVM event filter properly encapsulates core events.The
evmEventFilterfunction correctly defines the two core EVM events (BlockExecutedandTransactionExecuted) using the chain-specific EVM contract address.api/api.go (6)
95-119: LGTM! Constructor properly wired with fee parameters indexer.The
NewBlockChainAPIconstructor correctly accepts and stores theFeeParametersIndexerfor use across API methods.
186-192: LGTM! Graceful fallback for fee parameter retrieval.Lines 187-190 implement a defensive fallback to
DefaultFeeParameters()when the indexer fails, logging a warning. This ensures transaction submission remains available even if fee parameters are temporarily unavailable, which is appropriate for this endpoint.
862-873: LGTM! Fee history correctly applies surge factor uniformly.Lines 864-869 retrieve fee parameters once before the loop and apply the surge-adjusted gas price uniformly to all blocks. The fallback to base
GasPriceon error prevents array length mismatches, addressing previous review concerns.
1071-1077: LGTM! GasPrice degrades gracefully on fee parameter failure.Lines 1072-1075 implement the recommended fallback to base gas price when fee parameters are unavailable, ensuring the endpoint remains responsive while logging the issue for observability.
1118-1124: LGTM! MaxPriorityFeePerGas mirrors GasPrice fallback behavior.Lines 1119-1122 implement consistent fallback logic matching the
GasPriceendpoint, ensuring wallet UX remains stable when fee parameters are temporarily unavailable.
89-89: LGTM! Fee parameters indexer field properly added.Line 89 adds the
feeParametersfield to enable surge-aware pricing across API methods.services/requester/requester.go (1)
453-475: LGTM! GetLatestEVMHeight correctly uses exported ReplaceAddresses.Line 456 properly calls the refactored
ReplaceAddressesfunction with the Cadence script and chain ID, aligning with the codebase-wide renaming.bootstrap/bootstrap.go (6)
5-5: LGTM!The
embedandcadenceimports are correctly added to support the embedded Cadence script and UFix64 type handling for surge factor retrieval.Also applies to: 13-13, 55-58
60-68: LGTM!The
FeeParametersfield is correctly added to theStoragesstruct with the appropriatestorage.FeeParametersIndexerinterface type, consistent with other indexer fields.
202-202: LGTM!The
FeeParametersindexer is correctly passed toNewEventIngestionEngine, enabling the ingestion engine to persist fee parameter updates from Flow events.
326-326: LGTM!The
FeeParametersindexer is correctly passed toNewBlockChainAPI, enabling the API to apply surge-adjusted gas prices in RPC responses and validation.
658-665: Verify unconditional overwrite of fee parameters is intentional.During bootstrap, the code always fetches the current network surge factor and stores it, overwriting any previously stored fee parameters. This means every gateway restart will reset stored fee parameters to the current network state.
This behavior appears intentional (sync to network on startup), but confirm:
- Historical fee parameter changes tracked during ingestion are preserved elsewhere, OR
- Overwriting on restart is acceptable because fee parameters are transient state
If the unconditional Store is correct, consider adding a log statement to indicate when fee parameters are being initialized/synced:
feeParameters := pebble.NewFeeParameters(store) currentFeeParams, err := getNetworkFeeParams(context.Background(), config, client, logger) if err != nil { return nil, nil, fmt.Errorf("failed to fetch current fees surge factor: %w", err) } +logger.Info().Msgf("bootstrapping fee parameters with surge factor: %d", currentFeeParams.SurgeFactor) if err := feeParameters.Store(currentFeeParams, batch); err != nil { return nil, nil, fmt.Errorf("failed to bootstrap fee parameters: %w", err) }
674-682: LGTM!The
Storagesstruct is correctly initialized with all indexers, including the newly addedFeeParametersfield.
| // Code generated by mockery v2.43.2. DO NOT EDIT. | ||
|
|
||
| package mocks | ||
|
|
||
| import ( | ||
| models "github.com/onflow/flow-evm-gateway/models" | ||
| mock "github.com/stretchr/testify/mock" | ||
|
|
||
| pebble "github.com/cockroachdb/pebble" | ||
| ) | ||
|
|
||
| // FeeParametersIndexer is an autogenerated mock type for the FeeParametersIndexer type | ||
| type FeeParametersIndexer struct { | ||
| mock.Mock | ||
| } | ||
|
|
||
| // Get provides a mock function with given fields: | ||
| func (_m *FeeParametersIndexer) Get() (*models.FeeParameters, error) { | ||
| ret := _m.Called() | ||
|
|
||
| if len(ret) == 0 { | ||
| panic("no return value specified for Get") | ||
| } | ||
|
|
||
| var r0 *models.FeeParameters | ||
| var r1 error | ||
| if rf, ok := ret.Get(0).(func() (*models.FeeParameters, error)); ok { | ||
| return rf() | ||
| } | ||
| if rf, ok := ret.Get(0).(func() *models.FeeParameters); ok { | ||
| r0 = rf() | ||
| } else { | ||
| if ret.Get(0) != nil { | ||
| r0 = ret.Get(0).(*models.FeeParameters) | ||
| } | ||
| } | ||
|
|
||
| if rf, ok := ret.Get(1).(func() error); ok { | ||
| r1 = rf() | ||
| } else { | ||
| r1 = ret.Error(1) | ||
| } | ||
|
|
||
| return r0, r1 | ||
| } | ||
|
|
||
| // Store provides a mock function with given fields: feeParameters, batch | ||
| func (_m *FeeParametersIndexer) Store(feeParameters *models.FeeParameters, batch *pebble.Batch) error { | ||
| ret := _m.Called(feeParameters, batch) | ||
|
|
||
| if len(ret) == 0 { | ||
| panic("no return value specified for Store") | ||
| } | ||
|
|
||
| var r0 error | ||
| if rf, ok := ret.Get(0).(func(*models.FeeParameters, *pebble.Batch) error); ok { | ||
| r0 = rf(feeParameters, batch) | ||
| } else { | ||
| r0 = ret.Error(0) | ||
| } | ||
|
|
||
| return r0 | ||
| } | ||
|
|
||
| // NewFeeParametersIndexer creates a new instance of FeeParametersIndexer. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. | ||
| // The first argument is typically a *testing.T value. | ||
| func NewFeeParametersIndexer(t interface { | ||
| mock.TestingT | ||
| Cleanup(func()) | ||
| }) *FeeParametersIndexer { | ||
| mock := &FeeParametersIndexer{} | ||
| mock.Mock.Test(t) | ||
|
|
||
| t.Cleanup(func() { mock.AssertExpectations(t) }) | ||
|
|
||
| return mock | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify the mock is exercised by tests.
This is an auto-generated mock for FeeParametersIndexer. Based on the AI summary, services/ingestion/engine_test.go should instantiate and stub this mock, but the previous review flagged that no tests were found using it.
Run the following script to confirm test coverage:
If no tests use this mock, either add test coverage for components that depend on FeeParametersIndexer (e.g., ingestion engine tests) or remove the mock from the generation config to avoid dead code.
🏁 Script executed:
#!/bin/bash
# Description: Verify that tests import and use the FeeParametersIndexer mock
echo "=== Tests importing mocks.FeeParametersIndexer ==="
rg -n 'mocks\.FeeParametersIndexer' --glob '*_test.go' -C2
echo -e "\n=== Tests stubbing Get() on any mock ==="
rg -nP '\.On\("Get"\)' --glob '*_test.go' -C1
echo -e "\n=== Tests stubbing Store() on any mock ==="
rg -nP '\.On\("Store"' --glob '*_test.go' -C1
echo -e "\n=== NewFeeParametersIndexer constructor usage ==="
rg -n 'NewFeeParametersIndexer' --glob '*_test.go' -C2Length of output: 187
Remove or test the unused FeeParametersIndexer mock
The mock in storage/mocks/FeeParametersIndexer.go isn’t imported or stubbed by any *_test.go (no occurrences of mocks.FeeParametersIndexer, .On("Get"), .On("Store") or NewFeeParametersIndexer), so it’s dead code. Either add tests covering this interface or update the mock generation config to exclude it.
🤖 Prompt for AI Agents
storage/mocks/FeeParametersIndexer.go lines 1-77: the generated mock
FeeParametersIndexer is unused and should be removed or exercised; either delete
this file from the repo (and regenerate mocks without this interface) or add
tests that import mocks.FeeParametersIndexer and call NewFeeParametersIndexer /
.On("Get", ...)/.On("Store", ...) to assert behavior, or update your mockery
generation config to exclude FeeParametersIndexer so it isn’t produced next
time.
| }) | ||
| }) | ||
|
|
||
| t.Run("gas price updated with surge factor multipler", func(t *testing.T) { |
There was a problem hiding this comment.
Fix typo in test name.
The test name contains a typo: "multipler" should be "multiplier".
Apply this diff:
- t.Run("gas price updated with surge factor multipler", func(t *testing.T) {
+ t.Run("gas price updated with surge factor multiplier", func(t *testing.T) {📝 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.
| t.Run("gas price updated with surge factor multipler", func(t *testing.T) { | |
| tests/e2e_web3js_test.go | |
| t.Run("gas price updated with surge factor multiplier", func(t *testing.T) { |
🤖 Prompt for AI Agents
In tests/e2e_web3js_test.go around line 126, the test name has a typo:
"multipler" should be "multiplier"; update the t.Run call's first argument to
"gas price updated with surge factor multiplier" so the test name is spelled
correctly.
1 participant
Closes: #863
Closes: #864
Description
The changes include
FlowFees.FeeParametersChangedeventssurgeFactorfield from the above event payloadeth_gasPriceto return the configuredgas-pricemultiplied with the latestsurgeFactoreth_maxPriorityFeePerGasto return the configuredgas-pricemultiplied with the latestsurgeFactoreth_feeHistoryto return the configuredgas-pricemultiplied with the latestsurgeFactoreth_sendRawTransactionto reject tx submissions with gas prices below the configuredgas-pricemultiplied with the latestsurgeFactorFor contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
New Features
Tests