-
Notifications
You must be signed in to change notification settings - Fork 39
Calculate fees independently in sync #556
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Warning Rate limit exceeded@neekolas has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 40 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThe changes integrate a fee calculation mechanism into the synchronization workflow. A new fee calculator parameter is added to the Changes
Sequence Diagram(s)sequenceDiagram
participant ReplicationServer
participant SyncServer
participant SyncWorker
participant FeeCalculator
ReplicationServer->>SyncServer: NewSyncServer(ctx, log, nodeRegistry, registrant, writerDB, FeeCalculator)
SyncServer->>SyncWorker: startSyncWorker(..., FeeCalculator)
Note over SyncWorker: Envelope received for processing
SyncWorker->>FeeCalculator: calculateFees(envelope)
FeeCalculator-->>SyncWorker: Return calculated fee (spendPicodollars)
SyncWorker->>SyncWorker: validateAndInsertEnvelope(envelope, spendPicodollars)
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (4)
pkg/server/server.go (1)
157-157: Consider reusing a singlefeeCalculatorinstance.
Re-initializing the fee calculator on each “NewReplicationServer” might be fine for smaller workloads, but you could reuse or cache a single instance globally or per server if rates don’t change frequently. This may reduce overhead if the creation process becomes more expensive later.pkg/sync/syncWorker.go (3)
61-61: EnsurefeeCalculatoris properly initialized.
When creating the worker, verify thatfeeCalculatorfully implements all required methods and is not nil. You might handle potential initialization failures here, preventing runtime errors down the line.Also applies to: 72-72
412-412: Offer help for the TODO regarding fetch from other nodes.
The comment indicates there’s a requirement to handle envelopes from other nodes. If you’d like, I can open a new issue or propose a high-level approach for multi-node fetching logic.
504-524: Refine fee calculation logic.
The newcalculateFeesmethod is well structured, but keep an eye on potential changes to the congestion model. TheTODOindicates a future enhancement to calculate real congestion rates, which might affect performance or fairness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/server/server.go(1 hunks)pkg/sync/server.go(2 hunks)pkg/sync/syncWorker.go(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test (Node)
- GitHub Check: Build pre-baked anvil-xmtpd
🔇 Additional comments (5)
pkg/sync/server.go (2)
7-7: No concerns about new import.
Importing thefeespackage here is consistent with the new fee-calculation functionality.
27-29: ValidatefeeCalculatorparameter.
Consider adding a safeguard to ensurefeeCalculatoris non-nil before passing it to the worker. A nil calculator might lead to runtime errors or undefined behavior later.pkg/sync/syncWorker.go (3)
11-16: Imports align with fee calculation usage.
These newly added imports (constants,currency,fees) are relevant for the fee-calculation logic and don’t appear to introduce conflicts.
38-38: Confirm concurrency-safety offeeCalculator.
SincesyncWorkeris expected to handle concurrent envelope processing, ensurefeeCalculatoris threadsafe or used in a threadsafe manner.
457-460: ValidatespendPicodollarsparameter.
If negative or zero fees are passed, it might indicate abnormal inputs or logic. Consider adding checks to prevent incorrect usage data from persisting to the database.
134c35d to
654520e
Compare
fe24e1b to
28eaabe
Compare
654520e to
134c35d
Compare
28eaabe to
1656af8
Compare
134c35d to
3fa2947
Compare
1656af8 to
e0f7223
Compare
3fa2947 to
165627a
Compare
e0f7223 to
fd3c241
Compare
fd3c241 to
31c27ff
Compare
31c27ff to
0bcc377
Compare

Summary by CodeRabbit
New Features
Refactor