-
Notifications
You must be signed in to change notification settings - Fork 10
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
Add fees to UnsignedOriginatorEnvelope #548
base: 02-21-add_fee_calculation_helpers
Are you sure you want to change the base?
Add fees to UnsignedOriginatorEnvelope #548
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 1 minutes and 6 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 ignored due to path filters (1)
📒 Files selected for processing (12)
WalkthroughThe pull request updates the protobuf generation script and enhances the envelope structure. The script in Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Envelope as UnsignedOriginatorEnvelope
Client->>+Envelope: BaseFee()
Note right of Envelope: Access proto.BaseFeePicodollars
Envelope-->>-Client: currency.PicoDollar (base fee)
Client->>+Envelope: CongestionFee()
Note right of Envelope: Access proto.CongestionFeePicodollars
Envelope-->>-Client: currency.PicoDollar (congestion fee)
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 (
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
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: 0
🧹 Nitpick comments (1)
pkg/envelopes/unsignedOriginator.go (1)
54-57
: LGTM! Consider adding tests.The
CongestionFee()
method is well-implemented, following the same pattern asBaseFee()
. Consider adding unit tests to verify both fee methods work correctly with various fee values.Would you like me to help generate unit tests for both fee methods?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
pkg/proto/openapi/xmtpv4/message_api/message_api.swagger.json
is excluded by!pkg/proto/**
pkg/proto/openapi/xmtpv4/message_api/misbehavior_api.swagger.json
is excluded by!pkg/proto/**
pkg/proto/xmtpv4/envelopes/envelopes.pb.go
is excluded by!**/*.pb.go
,!pkg/proto/**
📒 Files selected for processing (2)
dev/gen-protos
(1 hunks)pkg/envelopes/unsignedOriginator.go
(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build pre-baked anvil-xmtpd
- GitHub Check: Test (Node)
🔇 Additional comments (3)
dev/gen-protos (1)
4-4
: Verify the branch usage for protobuf generation.The script now generates protobuf definitions from the
nm/add-fees
branch instead ofmain
. Please ensure that:
- The branch contains stable protobuf definitions.
- There is a plan to merge these changes back to
main
to avoid depending on a feature branch.pkg/envelopes/unsignedOriginator.go (2)
6-6
: LGTM!The added import for the
currency
package is necessary for the new fee methods.
49-52
: LGTM!The
BaseFee()
method is well-implemented:
- Returns the correct type (
currency.PicoDollar
).- Safely accesses the proto field (nil check in constructor).
- Clear and focused implementation.
13eca3f
to
ce9b947
Compare
242069b
to
b21cb0d
Compare
ce9b947
to
1ec1c15
Compare
9a2fafe
to
4b7a5dc
Compare
1ec1c15
to
b727641
Compare
4b7a5dc
to
d977442
Compare
b727641
to
77b36c3
Compare
d977442
to
0078ca4
Compare
TL;DR
Added fee-related fields to message envelopes. Populates them using a
FixedRateFetcher
which uses hard-coded rates. This will have to do until we can get rates from a smart contract.What changed?
base_fee_picodollars
andcongestion_fee_picodollars
fields toUnsignedOriginatorEnvelope
target_originator
field toPayerEnvelope
nm/add-fees
branch instead ofmain
How to test?
./dev/gen-protos
to verify protobuf generation works with new branchtarget_originator
field is properly serialized/deserializedWhy make this change?
To support fee calculation and payment functionality in the messaging system, allowing for base fees and congestion-based pricing while maintaining proper message routing through originators.
Summary by CodeRabbit
Chores
New Features