chore: improve storage cost #266
Conversation
WalkthroughThe changes update the logic for distributing storage costs in the payout process: now, the native $KYVE token is used first to cover storage costs, with other coins only used if $KYVE is insufficient. Corresponding test assertions and comments are updated to reflect this new distribution method. Proto file comment indentation is also adjusted. Changes
Sequence Diagram(s)sequenceDiagram
participant System
participant calculatePayouts
participant KYVE
participant OtherCoins
System->>calculatePayouts: Initiate payout calculation
calculatePayouts->>KYVE: Attempt to cover storage cost using $KYVE
alt $KYVE sufficient
KYVE-->>calculatePayouts: Full storage cost covered
calculatePayouts->>calculatePayouts: Distribute remaining payout
else $KYVE insufficient
KYVE-->>calculatePayouts: Partial storage cost covered
calculatePayouts->>OtherCoins: Split remaining storage cost among other coins
OtherCoins-->>calculatePayouts: Cover remainder as possible
calculatePayouts->>calculatePayouts: Distribute remaining payout
end
calculatePayouts-->>System: Return final payout distribution
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ 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). (2)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
proto/kyve/delegation/v1beta1/tx.proto (1)
11-13: Minor cosmetic change unrelated to PR objective.This indentation change from 2 spaces to 3 spaces is purely cosmetic and doesn't relate to the storage cost distribution improvements described in the PR. Consider whether this formatting change is necessary or if it should be part of a separate formatting-focused PR.
x/bundles/keeper/logic_bundles.go (1)
293-331: Complex but correct remaining storage cost distribution.The logic correctly handles distributing remaining storage costs (after $KYVE contribution) among other coins. The implementation:
- Properly separates $KYVE from remaining coins
- Distributes remaining USD storage cost equally among coins
- Correctly converts USD amounts to each coin's denomination
- Uses
Min()to prevent over-charging when insufficient fundsThe early returns prevent processing when funds are exhausted, which is appropriate.
Consider extracting the storage cost calculation logic into separate helper functions to improve readability and testability of this complex mathematical logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
proto/kyve/delegation/v1beta1/tx.proto(1 hunks)x/bundles/keeper/keeper_suite_inflation_splitting_test.go(2 hunks)x/bundles/keeper/keeper_suite_valid_bundles_test.go(1 hunks)x/bundles/keeper/logic_bundles.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
x/bundles/keeper/keeper_suite_valid_bundles_test.go (1)
Learnt from: troykessler
PR: KYVENetwork/chain#211
File: x/stakers/keeper/exported_functions.go:334-342
Timestamp: 2024-12-20T08:52:36.443Z
Learning: The `GetValidatorPoolStakes()` method ensures that the effective stake cannot exceed the validator's total bonded tokens, so no additional check is necessary.
x/bundles/keeper/logic_bundles.go (1)
Learnt from: troykessler
PR: KYVENetwork/chain#244
File: x/multi_coin_rewards/keeper/exported.go:19-23
Timestamp: 2025-02-11T09:41:10.529Z
Learning: When handling errors in reward distribution, prefer returning all rewards as a fallback to prevent potential loss of funds, rather than restricting to native rewards only.
⏰ 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 / test
🔇 Additional comments (6)
x/bundles/keeper/logic_bundles.go (3)
260-291: Well-implemented $KYVE-first storage cost logic.The implementation correctly prioritizes native $KYVE tokens for storage costs as described in the PR objective. The logic properly:
- Calculates storage cost in USD terms
- Converts to $KYVE amount using coin weight and currency unit
- Takes the minimum of available $KYVE and required storage cost
- Handles the early return case when payout is exhausted
The mathematical calculations appear correct and the approach aligns with the PR's goal of increasing $KYVE token usage for storage fees.
313-320: Correct currency conversion mathematics.The currency conversion logic properly handles the complex calculation from USD storage cost to individual coin amounts:
- Correctly calculates currency unit as 10^decimals for base denomination
- Proper USD-to-coin conversion using weight
- Appropriate truncation to integer for token amounts
- Correctly skips $KYVE (already processed) and zero-weight coins
The mathematical approach is sound and handles the multi-currency storage cost distribution correctly.
322-331: Conservative approach prevents fund loss.The implementation uses
Min()to ensure storage rewards don't exceed available payout, which aligns with the retrieved learning about preventing fund loss. The early return whentotalPayout.IsZero()prevents potential issues in subsequent commission/delegation calculations.This conservative approach is appropriate given the critical nature of reward distribution logic.
x/bundles/keeper/keeper_suite_inflation_splitting_test.go (2)
1515-1529: Well-documented test updates reflecting new storage cost logic.The test assertions have been correctly updated to reflect the $KYVE-first storage cost approach. The comprehensive comments explaining the changes and preserving previous expected values for reference demonstrate good testing practices.
The new expected values correctly show:
- Higher $KYVE amounts for uploader commission (778 vs 748)
- Lower amounts for other coins (990 vs 1004 acoin, 1980 vs 1987 bcoin)
- Complementary changes in delegator rewards
This aligns perfectly with the PR objective of increasing $KYVE token usage for storage fees.
1634-1648: Consistent test updates for 100% inflation splitting scenario.The test assertions for the 100% inflation splitting scenario have been updated consistently with the same $KYVE-first approach. The documentation follows the same high-quality pattern as the 10% test case.
The expected value changes show the same behavior pattern:
- Higher $KYVE for uploader commission (2492 vs 2461)
- Lower amounts for other coins (same acoin/bcoin values as 10% test)
- Appropriate delegator reward adjustments
The consistency across different inflation scenarios demonstrates thorough testing of the new storage cost distribution logic.
x/bundles/keeper/keeper_suite_valid_bundles_test.go (1)
1960-1972: Updated test assertions correctly reflect the new storage cost distribution logic.The test comments and assertions have been properly updated to reflect the change where $KYVE is now prioritized for storage cost coverage. The comments clearly explain:
- Commission rewards: Higher $KYVE amounts for uploaders since $KYVE covers storage costs first
- Self delegation rewards: Lower $KYVE amounts for delegators since more $KYVE is allocated to commission rewards
- The "VALUES BEFORE" comments provide helpful context for the change
The updated expected values in the test assertions are consistent with this new distribution logic where native $KYVE tokens are used first to cover storage costs, with other coins only used if $KYVE is insufficient.
This PR used the native $KYVE coin first to pay for the storage cost and only uses the remaining coins if $KYVE is not enough. This results in uploaders receiving more $KYVE with which they can pay for Turbo storage fees and delegators receiving more foreign coins which they receive by staking $KYVE.
Summary by CodeRabbit
Bug Fixes
Style