feat: support multiple coins for funding #179
Conversation
WalkthroughThe updates primarily focus on transitioning from single to multiple coin types across various modules, enhancing flexibility in funding operations. This includes changes from simple numeric values to lists of coins in funding-related fields, and adjustments in message structures and logic to support these enhancements in the blockchain's handling of transactions and balances. Changes
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (7)
Files not summarized due to errors (1)
Files not reviewed due to errors (1)
Files skipped from review due to trivial changes (1)
Files skipped from review as they are similar to previous changes (2)
Additional Context UsedLanguageTool (27)
Additional comments not posted (8)
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 20
Out of diff range and nitpick comments (8)
x/funders/keeper/msg_server_defund_pool.go (1)
Line range hint
33-53: Add a check for zero defund amount to avoid unnecessary operations.+ if msg.Amounts.IsZero() { + return &types.MsgDefundPoolResponse{}, nil + }x/delegation/keeper/keeper_suite_test.go (1)
31-39: Clarify that direct modifications tofunding.AmountsPerBundlein tests might not reflect production behavior.x/funders/keeper/msg_server_update_params_test.go (1)
[!TIP]
Codebase VerificationThe output from the executed script shows various instances where
CoinWhitelistis used across the codebase, particularly in tests and keeper logic. However, there is no evidence of checks for uniqueness of entries in the whitelist during updates or additions, which was the concern raised in the review comment.The review comment suggested that the test "Update multiple coin whitelist entries" should include a check to ensure that all entries are unique and do not duplicate existing entries in the whitelist. The script output does not show any usage of a method or function that would check for such uniqueness, such as a
Containsmethod or similar logic.Given this information, it appears that the review comment is valid, and the test does not currently include a check for the uniqueness of whitelist entries. This could potentially allow duplicate entries in the whitelist, which would be a flaw in maintaining the integrity of the whitelist.
Conclusion
The review comment is correct in pointing out the absence of a check for the uniqueness of entries in the whitelist during updates. This is a potential issue that needs to be addressed to ensure the integrity of the whitelist is maintained.
- The test in
msg_server_update_params_test.goshould be updated to include a check for uniqueness of the whitelist entries.- Similar checks should be considered in other parts of the codebase where the whitelist is updated.
Analysis chain
Line range hint
288-331: The test "Update multiple coin whitelist entries" should include a check to ensure that all entries are unique and do not duplicate existing entries in the whitelist. This is crucial for maintaining the integrity of the whitelist.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all whitelist entries are unique in similar update operations across the codebase. rg --type go 'CoinWhitelist' | grep -v 'Contains'Length of output: 6320
x/funders/keeper/logic_funders_test.go (1)
Line range hint
37-89: Ensure consistent use of coin denominations and weights in test setup.The test setup initializes a whitelist of coins with varying denominations and weights. It's crucial to ensure that these values are consistent and meaningful across tests to avoid confusion and potential bugs. Consider standardizing these values or clearly documenting their intended use cases.
docs/static/openapi.yml (4)
249-249: Ensure the description forprotocol_self_delegation_unbondingis complete and accurate.The description seems to be truncated or incomplete. It would be beneficial to provide a full and clear description for better understanding and documentation quality.
257-257: Ensure the description forprotocol_delegation_unbondingis complete and accurate.Similar to the previous comment, the description appears to be incomplete. Providing a full description will enhance clarity and documentation quality.
4662-4664: Clarify the description formin_funding_multiple.The description for
min_funding_multipleseems to be cut off. Providing a complete sentence will improve clarity and ensure that the documentation is informative.
8643-8650: Clarify the description forscore.The description for
scoreseems to be cut off. Providing a complete sentence will improve clarity and ensure that the documentation is informative.
# Conflicts: # testutil/integration/helpers.go # testutil/integration/integration.go # x/delegation/keeper/keeper_suite_test.go # x/query/types/account.pb.go
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (8)
x/delegation/keeper/msg_server_undelegate_test.go (8)
42-42: Consider adding a comment explaining the purpose ofCreatePoolin the test setup.
42-42: Ensure that theUploadIntervalparameter is appropriate for the test scenario, as it affects the timing of operations.
42-42: Add a comment explaining the significance of theInflationShareWeightparameter in the context of this test.
42-42: Confirm that theVersionparameter in the pool creation is set to a meaningful value for the test scenario.
42-42: Validate theRuntimeparameter in the pool creation to ensure it matches the expected value for the test scenario.
42-42: Check the consistency of theStartKeyparameter in the pool creation with the expected setup for the test.
42-42: Ensure that theNameparameter in the pool creation is set appropriately for the test context.
42-42: Verify that theUploadIntervalparameter is set correctly for the test scenario, as it affects the timing of operations.
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (2)
x/funders/keeper/logic_funders_test.go (2)
159-159: Consider renamingT_KYVEtoi.Mfor clarity, asT_KYVEmight be misleading when used withi.BCoins().
553-553: Clarification needed: Why is the variable namedDummy1? Consider using a more descriptive name.
| of funders. Funders are special actors who provide liquidity to a pool | ||
| and basically pay for the rewards the validators earn for their work. | ||
| Funders would usually be stakeholders of the data that is being archived and | ||
| therefore have a strong interest in further archiving the data. Once a valid |
There was a problem hiding this comment.
It seems like there are too many consecutive spaces here.
- therefore have a strong interest in further archiving the data.
+ therefore have a strong interest in further archiving the data.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| therefore have a strong interest in further archiving the data. Once a valid | |
| therefore have a strong interest in further archiving the data. Once a valid |
|
|
||
| Currently, the KYVE protocol allows at most 50 funders per pool to limit | ||
| gas consumption. If the slots are full and a funder wants to join anyway he | ||
| has to fund more than the current lowest funder. By doing so the funds of the |
There was a problem hiding this comment.
Consider adding a comma after this introductory phrase.
- By doing so the funds of the
+ By doing so, the funds of theCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| has to fund more than the current lowest funder. By doing so the funds of the | |
| has to fund more than the current lowest funder. By doing so, the funds of the |
|
|
||
| ## Multiple Coin Funding | ||
|
|
||
| To ease the process of funding KYVE pools we introduced the concept of multiple |
There was a problem hiding this comment.
Possible missing comma found.
- To ease the process of funding KYVE pools we introduced the concept of multiple
+ To ease the process of funding KYVE pools, we introduced the concept of multipleCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| To ease the process of funding KYVE pools we introduced the concept of multiple | |
| To ease the process of funding KYVE pools, we introduced the concept of multiple |
This PR adjust the funding module so funders can fund pools in multiple coins.
Summary by CodeRabbit
Documentation
New Features
Refactor
Tests
Chores