Support multiple fee receiver accounts - #100
Conversation
janezpodhostnik
changed the title
|
Warning Review limit reached
Next review available in: 46 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughChangesFee receiver configuration and processing
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Server
participant FlowClient
participant FlowFees
participant Logger
Server->>FlowClient: Retrieve latest block
Server->>FlowClient: Execute get_fee_receivers
FlowClient->>FlowFees: Call getFeeReceiverAddresses()
FlowFees-->>FlowClient: Return receiver addresses
FlowClient-->>Server: Return Cadence result
Server->>Logger: Log validation result
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Are the dependency updates needed or accidentally part of this PR?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/api.go`:
- Around line 108-109: Make successful fee-receiver validation a prerequisite
for readiness: update api/api.go lines 108-109 so Run either waits for
validateFeeReceivers before serving or keeps the service unready and rejects
construction requests until it succeeds. In api/validate.go lines 45-56 and 76,
propagate malformed validation results and retry exhaustion to that
startup/readiness gate instead of only logging them. Document the finalized
readiness behavior in README.md lines 377-380.
In `@api/validate.go`:
- Around line 24-26: Move the DataAccessNodes.Client() call from before the
retry loop into the loop around the existing attempt logic, so each retry
selects an access node independently while preserving the five-attempt behavior.
In `@go.mod`:
- Around line 14-16: Update the flow-go dependency pin in go.mod from commit
2799d6025ac3 to a current commit that contains upstream onflow/flow-go#8632,
specifically at or after merge commit b73fea20b2529a55c2bc3c3abb59a2ad891bba15;
alternatively, merge the prerequisite commit before retaining this change.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bc3db4ce-6190-4d11-bef1-0c0fc45a3f3c
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
README.mdapi/api.goapi/construction_service.goapi/validate.goconfig/config.goconfig/config_test.gogo.modscript/cadence/scripts/get-fee-receivers.cdcscript/script.goscript/script_test.gostate/process.gostate/state.gotestnet.json
| s.feeAddrs = s.Chain.Contracts.FeeAddresses() | ||
| go s.validateFeeReceivers(ctx) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make successful fee-receiver validation a readiness requirement.
Run starts validation in a goroutine and then starts the HTTP server. If configuration omits an active receiver, ConstructionPreprocess can accept a construction request for a transfer to that receiver before validation completes. A malformed result or exhausted retries also ends validation with only a log entry, so the service can continue without successful validation.
api/api.go#L108-L109: wait for successful validation before serving, or keep the service unready and reject construction requests until validation succeeds.api/validate.go#L45-L56: propagate malformed-result failures to the startup or readiness gate.api/validate.go#L76-L76: propagate retry exhaustion to the startup or readiness gate.README.md#L377-L380: document the finalized readiness behavior.
📍 Affects 3 files
api/api.go#L108-L109(this comment)api/validate.go#L45-L56api/validate.go#L76-L76README.md#L377-L380
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/api.go` around lines 108 - 109, Make successful fee-receiver validation a
prerequisite for readiness: update api/api.go lines 108-109 so Run either waits
for validateFeeReceivers before serving or keeps the service unready and rejects
construction requests until it succeeds. In api/validate.go lines 45-56 and 76,
propagate malformed validation results and retry exhaustion to that
startup/readiness gate instead of only logging them. Document the finalized
readiness behavior in README.md lines 377-380.
| github.com/onflow/cadence v1.10.5 | ||
| github.com/onflow/crypto v0.25.4 | ||
| github.com/onflow/flow-go v0.48.1-evm-cache-block.0.20260518173711-5b9fa9c8352e | ||
| github.com/onflow/flow-go v0.50.1-0.20260731132755-2799d6025ac3 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
pr="$(gh api repos/onflow/flow-go/pulls/8632 --jq '{state, merged_at, merge_commit_sha, head_sha: .head.sha}')"
printf '%s\n' "$pr"
merge_sha="$(printf '%s' "$pr" | jq -r '.merge_commit_sha')"
test "$merge_sha" != "null"
gh api "repos/onflow/flow-go/compare/${merge_sha}...2799d6025ac3" \
--jq '{status, ahead_by, behind_by}'Repository: onflow/rosetta
Length of output: 370
Flow commit 2799d6025ac3 does not include upstream onflow/flow-go#8632.
onflow/flow-go#8632 merged at b73fea20b2529a55c2bc3c3abb59a2ad891bba15, but 2799d6025ac3 is 30 commits behind that merge commit. Use a current flow-go pin that contains the required merge, or merge the prerequisite before this change.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@go.mod` around lines 14 - 16, Update the flow-go dependency pin in go.mod
from commit 2799d6025ac3 to a current commit that contains upstream
onflow/flow-go#8632, specifically at or after merge commit
b73fea20b2529a55c2bc3c3abb59a2ad891bba15; alternatively, merge the prerequisite
commit before retaining this change.
|
Notes on the CodeRabbit review threads: go.mod pin — false positive: Dependency updates — intentional: this PR needs flow-go with core-contracts v1.10.4 (the fee-receiver getter); cadence/sdk bumps ride along. Access node per retry — valid, applied in aac270d. Note the shipped configs run a single access node per spork, so in practice this is a consistency fix (every other call site already picks via Readiness gate — not taking this one. The dangerous case (an on-chain receiver missing from config) already ends in There is a genuine small gap nearby though: after 5 failed attempts, validation gives up for good with a single log line. A flaky access node at startup plus a stale config would leave the server running with an unvalidated fee set indefinitely. Follow-up idea for @janezpodhostnik (not a blocker): after the quick retries, fall back to a slow poll instead of returning, and/or surface the validation state via |
|
Tip For best results, initiate chat on the files or code changes.
I withdraw the The dependency updates are necessary for The I also withdraw the readiness-gate concern. Older FlowFees deployments can lack The slow-poll or 🐇 ✏️ Learnings added
You are interacting with an AI system. |
3 participants
Testnet distributes transaction fee deposits across multiple receiver accounts
since the FlowFees concurrent fee collection upgrade (rolled out in tx
be210889dd26a320f530595bd369093e866e26c3941bf7a3d01f861db3eeda81). Rosettaonly recognized deposits to the FlowFees contract account as fees, so deposits
to the child fee accounts were misclassified as ordinary transfers.
Changes
feeAddrwith a set of fee addresses(
Contracts.FeeAddresses()): the FlowFees account plus a new.contracts.fee_receiversconfig list, documented in the README.testnet.json, verified against theon-chain
FlowFees.getFeeReceiverAddresses()getter.validateFeeReceivers) that fetches the on-chainreceiver list via a new
get-fee-receiversscript and exits with a fatalerror if any on-chain receiver is missing from the config, so future
receiver additions fail loudly instead of silently misclassifying fees.
Extra configured addresses are allowed, since they may be needed to
classify fees in historical blocks.
getFeeReceiverAddresses()getter the script uses.Notes
mainnet.jsonis unchanged andbehavior there is identical. Once the upgrade rolls out there, the startup
check will flag the new receivers and
mainnet.jsonjust needs afee_receiversentry.yet merged. Do not merge this PR until it lands and the pin is updated to a
master commit.
Related: onflow/flow-core-contracts#575, onflow/flow-core-contracts#615, onflow/flow-go#8632
Summary by CodeRabbit
New Features
Documentation
fee_receiverssetting, address examples, fee behavior, and validation.Bug Fixes