Add fee calculation helpers #537
Conversation
WalkthroughThis pull request introduces advanced currency handling and fee calculation functionality. A new high-precision currency type, PicoDollar, is defined with functions to convert between dollars, picodollars, and microdollars along with string representation. In addition, fee calculation logic is implemented through a Changes
Sequence Diagram(s)sequenceDiagram
participant C as Controller
participant FC as FeeCalculator
participant RF as RatesFetcher
C->>FC: CalculateBaseFee(messageTime, messageSize, storageDuration)
FC->>RF: GetRates(messageTime)
RF-->>FC: Return Rates
FC-->>C: Return base fee (PicoDollar)
C->>FC: CalculateCongestionFee(messageTime, congestionUnits)
FC->>RF: GetRates(messageTime)
RF-->>FC: Return Rates
FC-->>C: Return congestion fee (PicoDollar)
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)Error: can't load config: the Go language version (go1.23) used to build golangci-lint is lower than the targeted Go version (1.24) 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
⏰ Context from checks skipped due to timeout of 90000ms (4)
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
pkg/fees/fixedRates.go (1)
11-13: Add input validation for rates parameter.Consider adding validation to ensure the
ratesparameter is not nil to prevent potential nil pointer dereferences.func NewFixedRatesFetcher(rates *Rates) *FixedRatesFetcher { + if rates == nil { + panic("rates cannot be nil") + } return &FixedRatesFetcher{rates: rates} }pkg/currency/currency_test.go (3)
9-16: Add more test cases for currency conversion.Consider adding test cases for:
- Edge cases (0, very large numbers)
- Negative numbers
- Numbers with more decimal places
func TestConversion(t *testing.T) { + testCases := []struct { + name string + dollars float64 + expected PicoDollar + }{ + {"regular case", 1.25, 1250000000000}, + {"zero", 0, 0}, + {"large number", 1000000.50, 1000000500000000000}, + {"negative", -1.25, -1250000000000}, + {"many decimals", 1.23456789, 1234567890000}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + initial, err := FromDollars(tc.dollars) + require.NoError(t, err) + require.Equal(t, tc.expected, initial) + + converted := initial.toDollars() + require.Equal(t, tc.dollars, converted) + }) + } }
18-22: Add more test cases for string representation.Consider adding test cases for:
- Zero
- Negative numbers
- Numbers with trailing zeros
- Large numbers
func TestString(t *testing.T) { + testCases := []struct { + name string + dollars float64 + expected string + }{ + {"regular case", 1.25, "1.25"}, + {"zero", 0, "0.00"}, + {"negative", -1.25, "-1.25"}, + {"trailing zeros", 1.50, "1.50"}, + {"large number", 1000000.25, "1000000.25"}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + initial, err := FromDollars(tc.dollars) + require.NoError(t, err) + require.Equal(t, tc.expected, initial.String()) + }) + } }
24-28: Add more test cases for microdollars conversion.Consider adding test cases for:
- Zero
- Negative numbers
- Numbers that might cause overflow
- Numbers with more decimal places
func TestToMicroDollars(t *testing.T) { + testCases := []struct { + name string + dollars float64 + expected int64 + }{ + {"regular case", 1.25, 1250000}, + {"zero", 0, 0}, + {"negative", -1.25, -1250000}, + {"large number", 1000000.50, 1000000500000}, + {"many decimals", 1.23456789, 1234568}, // rounds to nearest microdollar + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + initial, err := FromDollars(tc.dollars) + require.NoError(t, err) + require.Equal(t, tc.expected, initial.ToMicroDollars()) + }) + } }pkg/fees/calculator.go (1)
9-15: Add input validation for ratesFetcher parameter.Consider adding validation to ensure the
ratesFetcherparameter is not nil to prevent potential nil pointer dereferences.func NewFeeCalculator(ratesFetcher IRatesFetcher) *FeeCalculator { + if ratesFetcher == nil { + panic("ratesFetcher cannot be nil") + } return &FeeCalculator{ratesFetcher: ratesFetcher} }pkg/fees/interface.go (1)
9-15: Add validation constraints in comments.Consider documenting the expected constraints for each field:
- Valid ranges (e.g., non-negative values)
- Units of measurement
- Any other business rules
// Rates containt the cost for each fee component at a given message time. // Values in the rates struct are denominated in USD PicoDollars type Rates struct { - MessageFee currency.PicoDollar // The flat per-message fee - StorageFee currency.PicoDollar // The fee per byte-day of storage - CongestionFee currency.PicoDollar // The fee per unit of congestion + MessageFee currency.PicoDollar // The flat per-message fee (must be non-negative) + StorageFee currency.PicoDollar // The fee per byte-day of storage (must be non-negative) + CongestionFee currency.PicoDollar // The fee per unit of congestion (must be non-negative) }pkg/currency/currency.go (3)
9-14: Add documentation for the PicoDollarsPerDollar constant.While the type is well-documented, the constant lacks documentation explaining its purpose and significance.
Add a comment above the constant:
// Picodollar is a type to represent currency with 12 decimal precision type PicoDollar int64 +// PicoDollarsPerDollar represents the number of picodollars in one dollar (1e12) const ( PicoDollarsPerDollar = 1e12 )
26-29: Consider making ToDollars method exported.The method name
toDollarsstarts with a lowercase letter, making it unexported, while other conversion methods likeToMicroDollarsare exported. This inconsistency might make the API less intuitive.Consider making the method exported for consistency:
-func (p PicoDollar) toDollars() float64 { +func (p PicoDollar) ToDollars() float64 { return float64(p) / PicoDollarsPerDollar }
31-34: Extract magic number and improve documentation.The method uses a magic number and could benefit from more detailed documentation explaining the conversion ratio.
Consider these improvements:
+const ( + MicroDollarsPerDollar = 1e6 +) + +// ToMicroDollars converts PicoDollars to MicroDollars. +// The conversion ratio is: +// 1 Dollar = 1e12 PicoDollars = 1e6 MicroDollars func (p PicoDollar) ToMicroDollars() int64 { - return int64(p / 1e6) + return int64(p / (PicoDollarsPerDollar / MicroDollarsPerDollar)) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pkg/currency/currency.go(1 hunks)pkg/currency/currency_test.go(1 hunks)pkg/fees/calculator.go(1 hunks)pkg/fees/fixedRates.go(1 hunks)pkg/fees/interface.go(1 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/fees/fixedRates.go (2)
5-9: LGTM! Clear documentation and focused design.The comments effectively explain the purpose and use case of the
FixedRatesFetcherstruct.
15-17: LGTM! Clean implementation of the IRatesFetcher interface.The method correctly implements the interface contract and follows Go conventions for unused parameters.
pkg/fees/interface.go (1)
17-21: LGTM! Well-documented and focused interface design.The interface follows Go conventions and its purpose is clearly documented.
pkg/currency/currency.go (2)
1-7: LGTM! Clean package structure and imports.The package name is appropriate, and imports are well-organized with no unnecessary dependencies.
9-38:❓ Verification inconclusive
Verify precision requirements for currency handling.
The implementation uses 12 decimal places of precision (picodollars). Please verify if this level of precision aligns with the system requirements and industry standards for currency handling.
🌐 Web query:
What is the standard precision for handling cryptocurrency transactions?💡 Result:
Cryptocurrency transaction precision varies significantly across different digital assets and fiat currencies, with standards shaped by both technical protocols and practical usability considerations. Here's a detailed breakdown:
Bitcoin (BTC) Precision
- Maximum precision: 8 decimal places (1 satoshi = 0.00000001 BTC) [10]
- Internal calculations: Up to 10 decimal places in APIs, though only 8 are usable on-chain [2]
- Display standards: Often rounded to 5 decimal places for user interfaces [2]
Ethereum (ETH) and ERC-20 Tokens
- Standard precision: 18 decimal places (1 wei = 0.000000000000000001 ETH)
- Gwei subunit: Commonly used for gas fees (1 gwei = 0.000000001 ETH) [10]
Fiat Currency Handling
- USD calculations:
- Internal API precision: 4 decimal places
- Display standard: 2 decimal places [2]
- EUR/GBP: Typically follow similar patterns to USD
Token Variability
- General range: Most tokens use 8–18 decimal places depending on their protocol design [8]
- Examples:
- Stablecoins like USDT: Often mirror fiat standards (2 displayed decimals)
- High-supply tokens (e.g., SHIB): May use full 18-decimal precision
Implementation Considerations
APIs vs On-Chain:
- Exchanges like Kraken perform internal calculations at higher precision than displayed (e.g., BTC at 10 decimals internally vs 5 displayed) [2]
- Blockchain networks enforce protocol-defined decimal limits
Rounding Practices:
- Transactions exceeding display decimals get truncated/rounded (e.g., $1.45406823 → $1.4541 in APIs) [2]
- Mismatches require alignment between local calculations and API specifications
This tiered approach balances technical accuracy with human readability while maintaining consistency across wallets, exchanges, and blockchain networks.
Citations:
- 1: https://www.getfocal.ai/blog/cryptocurrency-transaction-monitoring
- 2: https://support.kraken.com/hc/en-us/articles/201988998-Decimal-precision-for-API-calculations
- 3: https://www.arkoselabs.com/explained/guide-to-cryptocurrency-security/
- 4: https://complyadvantage.com/insights/cryptocurrency-compliance-best-practices/
- 5: https://www.chainalysis.com/blog/introduction-to-cryptocurrency-exchange-compliance-crypto-businesses-2024/
- 6: https://www.investopedia.com/terms/c/chain-transactions-cryptocurrency.asp
- 7: https://www.kychub.com/blog/crypto-transaction-monitoring/
- 8: https://linkd.academy/en-us/library/glossary/token-decimals/
- 9: https://www.deloitte.com/mt/en/Industries/technology/perspectives/mt-article-cryptocurrency-security-standard-CCSS.html
- 10: https://www.gemini.com/cryptopedia/satoshi-value-gwei-to-ether-to-wei-converter-eth-gwei
Confirm the chosen precision for PicoDollar conversion aligns with system and industry requirements.
- The file uses 12 decimal places (PicoDollars) for representing currency.
- Web sources indicate that cryptocurrency precisions vary widely (e.g., BTC uses 8 decimals and ETH uses 18 decimals), while fiat currencies often use far fewer decimals.
- Please verify that the 12-decimal scheme meets your intended precision for both internal calculations and external displays, and consider whether any adjustments or additional tests (e.g., for rounding accuracy and overflow safety) are necessary.
neekolas
force-pushed
the
02-21-add_fee_calculation_helpers
branch
2 times, most recently
from
029ba8f to
8832f00
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/fees/calculator.go (3)
18-31: Solid input validation.Checking for negative
messageSizeandstorageDurationDaysis good practice for error prevention. Consider clarifying how zero values should be treated in domain-specific documentation (if zero storage days are valid).
38-49: Effective overflow checks—consider modularizing.The overflow checks for storage fees are sensible. As an optional improvement, extracting these checks into a helper function could reduce repetition and improve clarity. If fee values can grow significantly, exploring
big.Intmay provide safer arithmetic.
68-73: Congestion fee overflow checks—centralize logic.Similar overflow handling is repeated here. Refactoring shared multiplication checks into a separate helper method could simplify updates if the logic evolves.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pkg/currency/currency.go(1 hunks)pkg/currency/currency_test.go(1 hunks)pkg/fees/calculator.go(1 hunks)pkg/fees/fixedRates.go(1 hunks)pkg/fees/interface.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/currency/currency_test.go
- pkg/currency/currency.go
- pkg/fees/interface.go
- pkg/fees/fixedRates.go
⏰ 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/fees/calculator.go (5)
1-9: Package and imports look good.The package name and imports are clear and minimal, aligning with Go conventions.
10-16: Well-structured constructor.Returning a pointer to
FeeCalculatoris appropriate. The code is concise and straightforward.
33-36: Good error handling on rate fetch.Returning the error immediately ensures clarity. You might consider wrapping the error to provide more context, but this is optional.
52-61: Validate congestion range.Restricting congestion to 0–100 seems reasonable in most scenarios. Double-check domain requirements to ensure this range meets all future needs.
Would you like me to scan the codebase for calls to
CalculateCongestionFeeto confirm no assumptions exceed 100%?
63-66: Consistent rate fetching.This mirrors the approach in
CalculateBaseFee, promoting consistency and preventing silent failures.
neekolas
force-pushed
the
02-21-add_fee_calculation_helpers
branch
2 times, most recently
from
41d4e40 to
d9eb111
Compare
neekolas
force-pushed
the
02-21-link_payer_to_gateway_envelopes
branch
2 times, most recently
from
7b21dab to
eb0b89a
Compare
neekolas
force-pushed
the
02-21-add_fee_calculation_helpers
branch
from
d9eb111 to
58be2dd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
pkg/fees/calculator_test.go (5)
12-26: Add documentation for test rates.Consider adding comments to explain what these rates represent and how they were chosen for testing purposes. This will help other developers understand the test scenarios better.
const ( + // Fixed rates in picodollars for testing purposes + // These values are chosen to make test calculations easy to verify RATE_MESSAGE_FEE = 100 RATE_STORAGE_FEE = 50 RATE_CONGESTION_FEE = 200 )
28-40: Enhance test coverage with table-driven tests.Consider converting this to a table-driven test to cover more scenarios:
- Zero message size
- Zero storage duration
- Maximum valid values
func TestCalculateBaseFee(t *testing.T) { - calculator := setupCalculator() - - messageTime := time.Now() - messageSize := int64(100) - storageDurationDays := int64(1) - - baseFee, err := calculator.CalculateBaseFee(messageTime, messageSize, storageDurationDays) - require.NoError(t, err) - - expectedFee := RATE_MESSAGE_FEE + (RATE_STORAGE_FEE * messageSize * storageDurationDays) - require.Equal(t, currency.PicoDollar(expectedFee), baseFee) + tests := []struct { + name string + messageSize int64 + storageDuration int64 + expectedFee currency.PicoDollar + expectError bool + }{ + { + name: "normal case", + messageSize: 100, + storageDuration: 1, + expectedFee: currency.PicoDollar(RATE_MESSAGE_FEE + (RATE_STORAGE_FEE * 100 * 1)), + }, + { + name: "zero message size", + messageSize: 0, + storageDuration: 1, + expectedFee: currency.PicoDollar(RATE_MESSAGE_FEE), + }, + { + name: "zero storage duration", + messageSize: 100, + storageDuration: 0, + expectedFee: currency.PicoDollar(RATE_MESSAGE_FEE), + }, + } + + calculator := setupCalculator() + messageTime := time.Now() + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + baseFee, err := calculator.CalculateBaseFee(messageTime, tt.messageSize, tt.storageDuration) + if tt.expectError { + require.Error(t, err) + return + } + require.NoError(t, err) + require.Equal(t, tt.expectedFee, baseFee) + }) + } }
42-53: Enhance test coverage with table-driven tests.Similar to the base fee test, consider converting this to a table-driven test to cover more scenarios:
- 0% congestion
- 100% congestion
- Edge cases around the valid range
func TestCalculateCongestionFee(t *testing.T) { - calculator := setupCalculator() - - messageTime := time.Now() - congestionPercent := int64(50) - - congestionFee, err := calculator.CalculateCongestionFee(messageTime, congestionPercent) - require.NoError(t, err) - - expectedFee := RATE_CONGESTION_FEE * congestionPercent - require.Equal(t, currency.PicoDollar(expectedFee), congestionFee) + tests := []struct { + name string + congestionPercent int64 + expectedFee currency.PicoDollar + expectError bool + }{ + { + name: "50% congestion", + congestionPercent: 50, + expectedFee: currency.PicoDollar(RATE_CONGESTION_FEE * 50), + }, + { + name: "no congestion", + congestionPercent: 0, + expectedFee: 0, + }, + { + name: "full congestion", + congestionPercent: 100, + expectedFee: currency.PicoDollar(RATE_CONGESTION_FEE * 100), + }, + } + + calculator := setupCalculator() + messageTime := time.Now() + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + congestionFee, err := calculator.CalculateCongestionFee(messageTime, tt.congestionPercent) + if tt.expectError { + require.Error(t, err) + return + } + require.NoError(t, err) + require.Equal(t, tt.expectedFee, congestionFee) + }) + } }
55-71: Add overflow test for congestion fee calculation.The overflow test only covers base fee calculation. Consider adding test cases for congestion fee overflow and more specific overflow scenarios.
func TestOverflow(t *testing.T) { - calculator := setupCalculator() - - messageTime := time.Now() - - // Test overflow in CalculateBaseFee - messageSize := math.MaxInt64 - storageDurationDays := math.MaxInt64 - - _, err := calculator.CalculateBaseFee( - messageTime, - int64(messageSize), - int64(storageDurationDays), - ) - require.Error(t, err) - require.Contains(t, err.Error(), "storage fee calculation overflow") + tests := []struct { + name string + testFunc func(*FeeCalculator) error + errorString string + }{ + { + name: "base fee storage overflow", + testFunc: func(c *FeeCalculator) error { + _, err := c.CalculateBaseFee( + time.Now(), + math.MaxInt64, + math.MaxInt64, + ) + return err + }, + errorString: "storage fee calculation overflow", + }, + { + name: "congestion fee overflow", + testFunc: func(c *FeeCalculator) error { + _, err := c.CalculateCongestionFee( + time.Now(), + 100, // Even valid congestion percent can overflow with large fee + ) + return err + }, + errorString: "congestion fee calculation overflow", + }, + } + + calculator := setupCalculator() + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := tt.testFunc(calculator) + require.Error(t, err) + require.Contains(t, err.Error(), tt.errorString) + }) + } }
73-85: Convert to table-driven test for consistency.The test covers essential cases but could be more consistent with other tests by using a table-driven approach.
func TestInvalidCongestionPercent(t *testing.T) { - calculator := setupCalculator() - - messageTime := time.Now() - - _, err := calculator.CalculateCongestionFee(messageTime, int64(101)) - require.Error(t, err) - require.Contains(t, err.Error(), "congestionPercent must be between 0 and 100") - - _, err = calculator.CalculateCongestionFee(messageTime, int64(-1)) - require.Error(t, err) - require.Contains(t, err.Error(), "congestionPercent must be between 0 and 100") + tests := []struct { + name string + congestionPercent int64 + }{ + { + name: "above maximum", + congestionPercent: 101, + }, + { + name: "below minimum", + congestionPercent: -1, + }, + } + + calculator := setupCalculator() + messageTime := time.Now() + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := calculator.CalculateCongestionFee(messageTime, tt.congestionPercent) + require.Error(t, err) + require.Contains(t, err.Error(), "congestionPercent must be between 0 and 100") + }) + } }pkg/fees/calculator.go (1)
10-16: Add nil check in constructor.Consider adding a nil check for the ratesFetcher parameter to prevent potential panics.
func NewFeeCalculator(ratesFetcher IRatesFetcher) *FeeCalculator { + if ratesFetcher == nil { + panic("ratesFetcher cannot be nil") + } return &FeeCalculator{ratesFetcher: ratesFetcher} }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pkg/currency/currency.go(1 hunks)pkg/currency/currency_test.go(1 hunks)pkg/fees/calculator.go(1 hunks)pkg/fees/calculator_test.go(1 hunks)pkg/fees/fixedRates.go(1 hunks)pkg/fees/interface.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/fees/fixedRates.go
- pkg/currency/currency_test.go
- pkg/currency/currency.go
- pkg/fees/interface.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (2)
pkg/fees/calculator.go (2)
18-50: Well-implemented fee calculation with proper safeguards!The implementation:
- Uses picodollars for precise calculations
- Has thorough input validation
- Includes overflow checks for large numbers
- Provides clear error messages
52-73: Well-implemented congestion fee calculation!The implementation:
- Properly validates congestion percentage range
- Includes overflow checks
- Uses picodollars for precise calculations
|
I will wait for the downstack PRs to resolve. These graphite layers are hard for me to review. |
neekolas
force-pushed
the
02-21-add_fee_calculation_helpers
branch
from
58be2dd to
0dc26f5
Compare
|
@mkysel I think you were looking while it was restacking. The diff should be small and incremental again now |
| messageSize int64, | ||
| storageDurationDays int64, |
There was a problem hiding this comment.
you can probably get away with uint32 for both
There was a problem hiding this comment.
We can, but then we immediately have to convert to multiply against the int64 picodollars
pkg/fees/calculator.go
Outdated
|
|
||
| func (c *FeeCalculator) CalculateCongestionFee( | ||
| messageTime time.Time, | ||
| congestionPercent int64, |
There was a problem hiding this comment.
you can get away with uint8 here
neekolas
force-pushed
the
02-21-add_fee_calculation_helpers
branch
2 times, most recently
from
f318974 to
46352ee
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/fees/calculator.go (3)
20-21: Document the rationale for using int64.While
uint32could be used for these parameters, usingint64aligns with thePicoDollartype and avoids additional type conversions during calculations. Consider adding a comment explaining this design decision.func (c *FeeCalculator) CalculateBaseFee( messageTime time.Time, + // Using int64 to align with PicoDollar type and avoid conversions messageSize int64, storageDurationDays int64,
54-54: Consider using uint8 for congestionPercent.Since
congestionPercentis validated to be between 0 and 100, usinguint8would be more type-safe and better express the intent.
68-68: Document the percentage calculation.The congestion fee calculation multiplies directly by the percentage without dividing by 100. If this is intentional (i.e., if
CongestionFeeis pre-scaled), please add a comment to clarify this design decision.+ // CongestionFee is pre-scaled, so we multiply directly by the percentage (0-100) result := rates.CongestionFee * currency.PicoDollar(congestionPercent)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pkg/currency/currency.go(1 hunks)pkg/currency/currency_test.go(1 hunks)pkg/fees/calculator.go(1 hunks)pkg/fees/calculator_test.go(1 hunks)pkg/fees/fixedRates.go(1 hunks)pkg/fees/interface.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/fees/fixedRates.go
- pkg/currency/currency.go
- pkg/currency/currency_test.go
- pkg/fees/calculator_test.go
- pkg/fees/interface.go
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Test (Node)
- GitHub Check: Build pre-baked anvil-xmtpd
🔇 Additional comments (2)
pkg/fees/calculator.go (2)
10-16: LGTM! Clean implementation with proper dependency injection.The struct definition and constructor follow Go best practices, making the code maintainable and testable.
18-50: LGTM! Robust implementation with proper validation and overflow protection.The implementation successfully addresses previous review feedback by including input validation and overflow protection. The calculation logic is sound and handles edge cases appropriately.
neekolas
force-pushed
the
02-21-add_fee_calculation_helpers
branch
from
46352ee to
13eca3f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/fees/interface.go (3)
9-11: Fix typo in documentation.There's a typo in the comment: "containt" should be "contains".
-// Rates containt the cost for each fee component at a given message time. +// Rates contains the cost for each fee component at a given message time.
23-23: Add interface documentation.The interface lacks documentation explaining its purpose and methods. Consider adding documentation that describes:
- The purpose of the interface
- When to use each method
- The meaning of the parameters
- The expected behavior when errors occur
+// IFeeCalculator calculates various types of fees for messages. +// It uses high-precision PicoDollar for all fee calculations to avoid floating-point errors. type IFeeCalculator interface {
29-33: Improve parameter naming for clarity.The parameter name
congestionUnitsis not self-explanatory. Consider using a more descriptive name that indicates what these units represent and how they affect the fee calculation.CalculateCongestionFee( messageTime time.Time, - congestionUnits int64, + congestionPercent int64, ) (currency.PicoDollar, error)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pkg/currency/currency.go(1 hunks)pkg/currency/currency_test.go(1 hunks)pkg/fees/calculator.go(1 hunks)pkg/fees/calculator_test.go(1 hunks)pkg/fees/fixedRates.go(1 hunks)pkg/fees/interface.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/fees/fixedRates.go
- pkg/currency/currency_test.go
- pkg/fees/calculator_test.go
- pkg/currency/currency.go
- pkg/fees/calculator.go
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Build pre-baked anvil-xmtpd
- GitHub Check: Test (Node)
🔇 Additional comments (2)
pkg/fees/interface.go (2)
17-21: Well-designed interface with clear documentation!The interface follows good design principles:
- Single responsibility
- Clear documentation explaining the rationale
- Proper error handling
24-28: Fix parameter type inconsistency with implementation.The interface defines
messageSizeandstorageDurationDaysasint64, but the implementation usesint. This mismatch could cause issues.
neekolas
force-pushed
the
02-21-link_payer_to_gateway_envelopes
branch
from
eb0b89a to
ee5308f
Compare
neekolas
force-pushed
the
02-21-add_fee_calculation_helpers
branch
from
13eca3f to
ce9b947
Compare
neekolas
force-pushed
the
02-21-add_fee_calculation_helpers
branch
from
ce9b947 to
1ec1c15
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/testutils/envelopes/envelopes.go (1)
107-107: Consider consistent usage ofTargetOriginatoracross envelope creation functions.
At line 76,TargetOriginatoris assigned vianew(uint32), whereas here it is assigned by value (nodeID). This inconsistency might lead to confusion in future maintenance or usage of these envelopes.pkg/fees/calculator.go (2)
10-16: Add doc comments for clarity.
Consider adding brief doc comments for both the struct and constructor to enhance code readability and maintainability.
52-77: Align the error message with the variable name.
The variable is namedcongestionUnits, but the error message refers to it ascongestionPercent. Update the error message to avoid confusion:- "congestionPercent must be between 0 and 100, got %d" + "congestionUnits must be between 0 and 100, got %d"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
pkg/currency/currency.go(1 hunks)pkg/currency/currency_test.go(1 hunks)pkg/fees/calculator.go(1 hunks)pkg/fees/calculator_test.go(1 hunks)pkg/fees/fixedRates.go(1 hunks)pkg/fees/interface.go(1 hunks)pkg/testutils/envelopes/envelopes.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/fees/fixedRates.go
- pkg/fees/calculator_test.go
- pkg/fees/interface.go
- pkg/currency/currency.go
- pkg/currency/currency_test.go
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Test (Node)
- GitHub Check: Build pre-baked anvil-xmtpd
🔇 Additional comments (2)
pkg/fees/calculator.go (2)
1-9: Introduced package and imports look fine.
No issues or improvements needed here.
18-50: Base fee calculation appears robust.
The checks for negative inputs and overflow are appropriate, and returning an error when overflow is detected is a sensible approach.
neekolas
force-pushed
the
02-21-link_payer_to_gateway_envelopes
branch
from
ee5308f to
8d666c7
Compare
neekolas
force-pushed
the
02-21-add_fee_calculation_helpers
branch
from
1ec1c15 to
b727641
Compare
| "congestionPercent must be between 0 and 100, got %d", | ||
| congestionUnits, |
There was a problem hiding this comment.
nit: consider renaming congestionUnits to congestionPercent everywhere, so the expected input is clear.
There was a problem hiding this comment.
You and @mkysel will have to hash that one out. His suggestion was to switch from percent to units, since we don't divide it by 100
There was a problem hiding this comment.
haha :) yeah! I guess the text of the error message should match the name of the variable
neekolas
force-pushed
the
02-21-add_fee_calculation_helpers
branch
from
b727641 to
77b36c3
Compare
TL;DR
Notes
Handling money in integers feels like the right way to do it, even if we have to convert from PicoDollars to MicroDollars (6 decimals) when we go onchain.
We will have to be careful about that conversion to make sure no one can get free messaging by abusing the rounding.
What changed?
currencypackage withPicoDollartype for precise financial calculationsHow to test?
Why make this change?
To provide a precise and reliable fee calculation system that:
Summary by CodeRabbit
New Features
Tests