chore: prepare v2 upgrade #245
Conversation
WalkthroughThis update modifies multiple modules to enhance upgrade handling, staker queries, and the migration of merkle_root bundles. It adjusts configuration metadata, updates dependency versions and Go support, and modifies the upgrade process by incorporating new keeper parameters. Changes span documentation updates (CHANGELOG.md, SECURITY.md), build metadata (Makefile), application logic (app/app.go, app/upgrades/.../upgrade.go), and dependency management (go.mod, interchaintest/go.mod). Additionally, new JSON files for validator configurations are introduced. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant UK as UpgradeKeeper
participant UH as UpgradeHandler
participant GK as GlobalKeeper
participant MCRK as MultiCoinRewardsKeeper
App->>UK: SetUpgradeHandler(v2_0, handler with new keepers)
UK->>UH: CreateUpgradeHandler(..., GK, MCRK)
UH->>GK: AdjustGasConfig & set rewards params
UH->>MCRK: SetMultiCoinRewardsParams for reward distribution
Note right of UH: Upgrade process now integrates global and multi-coin rewards functionalities.
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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 (
|
![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 (3)
app/upgrades/v2_0/upgrade.go (3)
80-86: Consider externalizing or documenting the hard-coded admin address.
Hard-coded parameters (like the admin address) can limit flexibility. Consider making them configurable via on-chain governance or configs.
88-116: AdjustGasConfig function is a helpful addition but may need more configurability.
While setting the min gas price and message-specific gas adjustments here makes sense, consider referencing these values from a configuration or parameter file, so that changes can be managed without code redeployment.
142-145: Chain-specific validator mapping could be more scalable.
The approach of conditional logic for mainnet vs. kaon is straightforward, but consider a more extensible or config-based approach for different chain IDs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
go.sumis excluded by!**/*.sumgo.workis excluded by!**/*.workgo.work.sumis excluded by!**/*.suminterchaintest/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
CHANGELOG.md(1 hunks)Makefile(2 hunks)SECURITY.md(1 hunks)app/app.go(1 hunks)app/upgrades/v2_0/upgrade.go(5 hunks)go.mod(10 hunks)interchaintest/go.mod(11 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Summary
- GitHub Check: test / test
- GitHub Check: lint / golangci
🔇 Additional comments (18)
app/upgrades/v2_0/upgrade.go (7)
7-7: Import for multicoin rewards keeper looks good.
It is used for the newly integrated multi-coin rewards functionality.
15-15: Import for math package is appropriate.
Used to handle decimal operations for parameters.
17-17: Import for global keeper is correct.
This import is used for retrieving and setting global chain parameters.
50-51: Additional keepers in theCreateUpgradeHandlersignature.
Expanding the signature withglobalKeeperandmultiCoinRewardsKeeperis consistent with the new multi-coin and global param logic.
63-66: Confirm the necessity of creating two module accounts.
You're callingEnsureMultiCoinDistributionAccounttwice with different names, then adjusting gas config. Verify that both accounts are indeed required and will not conflict.
67-67: Invoking SetMultiCoinRewardsParams is valid.
The call to configure multi-coin rewards parameters during upgrade is clearly placed.
118-119: Ensure consistent module account initialization.
Forcibly converting an existing account into a module account resets its sequence to zero. Confirm that this won't disrupt any active sequences or signing.Also applies to: 124-125, 129-130
app/app.go (1)
418-419: Updated upgrade handler call is properly synced with the new signature.
Passingapp.GlobalKeeperandapp.MultiCoinRewardsKeeperaligns with the expanded function parameters.SECURITY.md (1)
8-9: Updated version support is consistent with the major release.
Replacingv1.4.xsupport withv1.5.xreflects the current maintenance policy.Makefile (3)
5-5: LGTM! Version bump to v2.0.0.The version bump from v1.5.0 to v2.0.0 aligns with the PR's objective to prepare for a v2 upgrade.
7-7: Verify the build timestamp.The build time is set to February 10, 2025, which is in the future. Please ensure this is intentional or adjust it to a more appropriate timestamp.
33-34: LGTM! Added binary size optimization.The addition of
-w -slinker flags will strip debug information and symbol table entries, reducing the final binary size.CHANGELOG.md (2)
21-21: LGTM! State machine breaking change documented.The merkle_root bundles migration is correctly marked as a state machine breaking change with "!" and includes a reference to PR #236.
28-28: LGTM! Stakers query update documented.The update to stakers queries is properly documented under improvements with a reference to PR #232.
interchaintest/go.mod (2)
65-66: LGTM! Core dependency updates.Updated core dependencies to their latest stable versions:
- cometbft v0.38.17
- cometbft-db v0.14.1
- OpenTelemetry packages to v1.32.0
Also applies to: 338-341
3-3:✅ Verification successful
Verify Go version compatibility.
The Go version has been updated to 1.22.11. Please ensure all CI/CD pipelines and development environments are updated accordingly.
Run the following script to check the Go version in use:
🏁 Script executed:
#!/bin/bash # Description: Check Go version in use across the codebase. # Check Go version in CI/CD configuration files echo "Checking CI/CD files for Go version..." rg -A 5 "go-version|golang-version|GO_VERSION"Length of output: 3317
Action Required: Confirm CI/CD and Local Environments Use Go 1.22.x
The update in
interchaintest/go.modnow specifiesgo 1.22.11, which is compatible with the CI/CD and Makefile requirements (the Makefile extracts and checks for the base version1.22). Please verify that both your CI/CD pipelines and local development environments are configured to use a Go version in the 1.22.x series to ensure smooth builds and tests.
- Location:
interchaintest/go.mod– Go version updated to1.22.11- Location:
Makefile– Confirms environment must report as1.22go.mod (2)
3-3: Verify Go version consistency.The Go version has been updated to 1.22.11, matching the version in interchaintest/go.mod.
Please ensure this version is documented in:
- Development setup guides
- Contribution guidelines
- Docker build configurations
376-376:❓ Verification inconclusive
Verify KYVE-flavored Cosmos SDK compatibility.
The replacement directive points to a KYVE-specific fork of Cosmos SDK v0.50.11-kyve-rc2.
Run the following script to check the compatibility:
🏁 Script executed:
#!/bin/bash # Description: Verify Cosmos SDK fork compatibility. # Check if the fork exists and is up to date gh api repos/KYVENetwork/cosmos-sdk/releases/tags/v0.50.11-kyve-rc2 # Check for any open issues related to the fork gh api search/issues -f q='repo:KYVENetwork/cosmos-sdk is:issue is:open label:bug'Length of output: 509
Manual Verification Needed: KYVE Fork Release Not Found
The GH CLI commands indicate that the release tag
v0.50.11-kyve-rc2is not found and the issue search query returned a validation error. Please manually verify the following:
- Confirm that the KYVE-flavored Cosmos SDK fork at
github.com/KYVENetwork/cosmos-sdkis available and that the intended release/tagv0.50.11-kyve-rc2exists.- Check whether the dependency should instead point to a different release, branch, or directly to a commit identifier.
- Revisit the issue search query syntax if automated checks are desired, as the current query appears to be malformed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/upgrades/v2_0/upgrade.go (1)
99-131: Verify gas configuration values.The gas configuration includes:
- Minimum gas price set to 2.
- Gas adjustments for validator and funder creation.
- Gas refunds for bundle operations.
Please verify these values align with the intended economic model.
Consider documenting the rationale behind these specific gas values in the code comments or documentation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/app.go(1 hunks)app/upgrades/v2_0/upgrade.go(5 hunks)app/upgrades/v2_0/validator-proofs/korellia/Validator-EU.json(1 hunks)app/upgrades/v2_0/validator-proofs/korellia/Validator-OVH.json(1 hunks)app/upgrades/v2_0/validator-proofs/korellia/Validator-US.json(1 hunks)app/upgrades/v2_0/validator_mapping.go(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- app/upgrades/v2_0/validator-proofs/korellia/Validator-EU.json
- app/upgrades/v2_0/validator-proofs/korellia/Validator-US.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (7)
app/upgrades/v2_0/validator_mapping.go (1)
37-37: LGTM!The addition of
ValidatorMappingsKorelliafollows the existing pattern and is properly integrated into the migration logic.Also applies to: 51-52
app/upgrades/v2_0/upgrade.go (4)
72-72: TODO: Address the withdraw address configuration.Please ensure this TODO is addressed before the upgrade is finalized.
94-94: TODO: Address the mainnet inflation split configuration.Please ensure this TODO is addressed before the upgrade is finalized.
83-89: Verify multi-coin rewards parameters.The configuration sets:
- Pending time to 14 days.
- Policy admin address to a specific KYVE Public Good Funding address.
Please verify:
- The pending time of 14 days is appropriate for the use case.
- The policy admin address is correct and secure.
112-129: Verify gas refund configuration for bundle operations.A past review comment mentioned that
kyve.bundles.v1beta1.ClaimUploaderRolewas missing. However, it's now included in the gas refunds configuration.app/app.go (1)
418-420: LGTM!The addition of GlobalKeeper, MultiCoinRewardsKeeper, and PoolKeeper to the upgrade handler is consistent with the changes in the upgrade implementation.
app/upgrades/v2_0/validator-proofs/korellia/Validator-OVH.json (1)
1-7:✅ Verification successful
Verify validator configuration.
Please verify:
- The consensus and protocol addresses are correct for the OVH validator.
- The proof values are valid and properly generated.
Run the following script to verify the address formats:
🏁 Script executed:
#!/bin/bash # Description: Verify the validator addresses follow the correct format # Test: Check if the consensus address starts with "kyvevaloper" if ! grep -q '"consensus_address": "kyvevaloper' app/upgrades/v2_0/validator-proofs/korellia/Validator-OVH.json; then echo "Error: Consensus address should start with 'kyvevaloper'" exit 1 fi # Test: Check if the protocol address starts with "kyve1" if ! grep -q '"protocol_address": "kyve1' app/upgrades/v2_0/validator-proofs/korellia/Validator-OVH.json; then echo "Error: Protocol address should start with 'kyve1'" exit 1 fi # Test: Check if proof values are valid hex strings if ! grep -E '"proof_[12]": "[0-9A-F]{64}"' app/upgrades/v2_0/validator-proofs/korellia/Validator-OVH.json; then echo "Error: Proof values should be 64-character hex strings" exit 1 fiLength of output: 475
OVH Validator Configuration Verified
The validator configuration for OVH has been confirmed:
- Consensus Address: Correctly starts with "kyvevaloper".
- Protocol Address: Correctly starts with "kyve1".
- Proof Values: Both proof values are valid 64-character hexadecimal strings.
No issues were found.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/upgrades/v2_0/upgrade.go (3)
76-76: Address the TODO comment about setting withdraw address.The TODO comment indicates that withdraw address setup is pending. This should be addressed before the upgrade.
Would you like me to help implement the withdraw address setup or create an issue to track this task?
96-154: Consider extracting IBC denoms to constants or configuration.The function hardcodes IBC denoms and pool weights. Consider moving these values to constants or a configuration file for better maintainability.
162-162: Address the TODO comment about mainnet inflation split.The TODO comment indicates that mainnet inflation split configuration is pending. This should be addressed before the upgrade.
Would you like me to help implement the mainnet inflation split configuration or create an issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/upgrades/v2_0/upgrade.go(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test / test
- GitHub Check: Summary
🔇 Additional comments (4)
app/upgrades/v2_0/upgrade.go (4)
6-19: LGTM! Imports are well-organized.The new imports for poolkeeper, multicoinrewardskeeper, globalkeeper, and math are properly organized and utilized throughout the code.
42-85: LGTM! Upgrade handler is well-structured.The upgrade handler properly integrates the new keepers and executes the necessary migration steps in a logical order.
167-199: LGTM! Gas configuration is comprehensive.The gas configuration properly includes all necessary message types including the previously missing
ClaimUploaderRole.
201-286: LGTM! Migration functions are robust and well-documented.The migration functions properly handle account creation/updates and staker migration with appropriate error handling and logging.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/upgrades/v2_0/upgrade.go (1)
180-197:⚠️ Potential issueAdd missing ClaimUploaderRole gas refund.
The gas refunds list is missing the
kyve.bundles.v1beta1.ClaimUploaderRoleentry that was mentioned in past reviews.Add the missing gas refund entry:
params.GasRefunds = []globalTypes.GasRefund{ { Type: "/kyve.bundles.v1beta1.SubmitBundleProposal", Fraction: math.LegacyMustNewDecFromStr("0.99"), }, + { + Type: "/kyve.bundles.v1beta1.ClaimUploaderRole", + Fraction: math.LegacyMustNewDecFromStr("0.99"), + }, // ... other refunds }
🧹 Nitpick comments (3)
app/upgrades/v2_0/upgrade.go (3)
76-76: Address the TODO comment about setting withdraw address.This TODO comment should be addressed before the upgrade to ensure all necessary configurations are in place.
Would you like me to help implement the withdraw address setting functionality?
91-91: Consider making the policy admin address configurable.The hardcoded policy admin address should be moved to a configuration parameter for better maintainability.
162-162: Address the TODO comment about mainnet inflation split.This TODO comment should be addressed before the upgrade to ensure proper inflation parameters.
Would you like me to help implement the mainnet inflation split configuration?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/upgrades/v2_0/upgrade.go(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test / test
- GitHub Check: Summary
🔇 Additional comments (2)
app/upgrades/v2_0/upgrade.go (2)
6-8: LGTM! Well-organized imports and keeper additions.The new imports and keeper parameters are properly organized and follow Go conventions.
Also applies to: 18-18, 51-53
201-215: LGTM! Well-implemented migration functions.The migration functions are properly implemented with:
- Proper error handling
- Informative logging
- Clear state transitions
Also applies to: 217-286
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/upgrades/v2_0/upgrade.go (1)
184-216:⚠️ Potential issueAdd missing ClaimUploaderRole message type.
The gas configuration is missing the previously mentioned
kyve.bundles.v1beta1.ClaimUploaderRolemessage type.Apply this diff to add the missing message type:
params.GasAdjustments = []globalTypes.GasAdjustment{ { Type: "/cosmos.staking.v1beta1.MsgCreateValidator", Amount: 50000000, }, { Type: "/kyve.funders.v1beta1.MsgCreateFunder", Amount: 50000000, }, + { + Type: "/kyve.bundles.v1beta1.ClaimUploaderRole", + Amount: 50000000, + }, }
🧹 Nitpick comments (3)
app/upgrades/v2_0/upgrade.go (3)
76-79: Track and implement TODO items before the upgrade.Several critical TODO items need to be addressed:
- Setting withdraw address
- Keeping distribution params for gov proposals
- Migrating stakers type URL
- Adding runtime upgrades
These items appear to be essential for the upgrade process.
Would you like me to help create tracking issues for these TODO items or assist with their implementation?
90-174: Add validation for IBC denoms and pool IDs.The multi-coin rewards configuration is comprehensive but could benefit from additional validation:
- Verify that the IBC denoms exist and are valid
- Ensure pool IDs are within valid range
- Consider adding comments explaining the significance of each IBC denom
Let's verify the IBC denoms:
#!/bin/bash # Description: Verify IBC denoms exist in the chain for denom in \ "ibc/A59C9E368C043E72968615DE82D4AD4BC88E34E6F353262B6769781C07390E8A" \ "ibc/F4E5517A3BA2E77906A0847014EBD39E010E28BEB4181378278144D22442DB91" \ "ibc/D0C5DCA29836D2FD5937714B21206DD8243E5E76B1D0F180741CCB43DCAC1584" \ "ibc/506478E08FB0A2D3B12D493E3B182572A3B0D7BD5DCBE71610D2F393DEDDF4CA" \ "ibc/7D5A9AE91948931279BA58A04FBEB9BF4F7CA059F7D4BDFAC6C3C43705973E1E"; do echo "Checking denom: $denom" rg -l "$denom" done
234-303: Enhance error handling in staker migration.While the migration logic is comprehensive, consider adding error handling for:
- Invalid validator mappings
- Failed delegations (currently only panics)
- Commission reward transfers
Consider wrapping critical operations in a try-catch block and adding rollback mechanisms for atomic updates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/upgrades/v2_0/upgrade.go(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (2)
app/upgrades/v2_0/upgrade.go (2)
6-8: LGTM! Well-organized keeper imports and handler signature update.The new imports and parameter additions to
CreateUpgradeHandlerare logically organized and follow a consistent pattern.Also applies to: 18-18, 51-53
218-232: LGTM! Robust account management implementation.The account management implementation is well-structured with:
- Proper error handling
- Clear distinction between new account creation and existing account adjustment
- Flexible naming through parameterization
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/upgrades/v2_0/upgrade.go (1)
116-168: Consider making IBC denoms configurable.The IBC denoms are currently hardcoded in the upgrade handler. This could be problematic if the denoms change or if new chains need to be supported.
Consider moving these values to a configuration file or making them part of the chain's parameters.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/upgrades/v2_0/upgrade.go(5 hunks)x/delegation/module.go(1 hunks)x/delegation/types/codec.go(1 hunks)x/stakers/types/codec.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (5)
app/upgrades/v2_0/upgrade.go (2)
77-80: Address critical TODO items before upgrade.These TODO items appear to be critical for the upgrade process. Please ensure these tasks are completed:
- Setting withdraw address
- Keeping distribution params for gov proposals
- Migrating stakers type URL
- Adding runtime upgrades
Could you provide more details about the implementation plan for these tasks?
180-180: Address mainnet inflation split TODO.The TODO comment indicates that mainnet inflation split configuration is missing.
Please clarify the intended inflation split configuration for mainnet.
x/delegation/types/codec.go (1)
5-5: LGTM! Interface registration follows Cosmos SDK patterns.The implementation correctly registers
MsgUpdateParamsas an implementation ofsdk.Msg, which is essential for message handling in the v2 upgrade.Also applies to: 10-12
x/stakers/types/codec.go (1)
4-4:✅ Verification successful
Verify potential naming conflicts between MsgUpdateParams versions.
While the registration of both current and v1beta1 versions of
MsgUpdateParamsis correct for upgrade compatibility, ensure that message routing and handling logic can properly distinguish between these versions.Let's check for potential naming conflicts in message handling:
Also applies to: 25-25
🏁 Script executed:
#!/bin/bash # Search for MsgUpdateParams handler implementations rg -A 5 "MsgUpdateParams" --type goLength of output: 95486
Ensure Distinct Handling of MsgUpdateParams Versions
The grep results confirm that the current and v1beta1 versions of
MsgUpdateParamsare registered using separate package aliases—most notably, with the v1beta1 version clearly imported astypes_v1beta1. Handler implementations and tests consistently reference the appropriate type, indicating that no naming conflict exists at the registration or routing level.
- In x/stakers/types/codec.go, both versions are registered distinctly.
- Handler implementations (e.g., in x/stakers/keeper/msg_server_update_params.go) and associated tests explicitly refer to the intended types, ensuring unambiguous message routing.
Please verify in integration tests that the message dispatch continues to distinguish between the two versions correctly during runtime.
x/delegation/module.go (1)
65-65: LGTM! Interface registration properly integrated.The addition of
types.RegisterInterfaces(reg)correctly integrates the codec registration with the module setup.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
app/upgrades/v2_0/upgrade.go (6)
3-36: Consider organizing imports for better readability.Consider grouping imports in the following order:
- Standard library
- Third-party packages (cosmos-sdk)
- Internal packages (KYVE)
import ( "context" "fmt" - poolTypes "github.com/KYVENetwork/chain/x/pool/types" - - poolkeeper "github.com/KYVENetwork/chain/x/pool/keeper" - - multicoinrewardskeeper "github.com/KYVENetwork/chain/x/multi_coin_rewards/keeper" - multicoinrewardstypes "github.com/KYVENetwork/chain/x/multi_coin_rewards/types" - - authkeeper "github.com/cosmos/cosmos-sdk/x/auth/keeper" - authTypes "github.com/cosmos/cosmos-sdk/x/auth/types" - - bundleskeeper "github.com/KYVENetwork/chain/x/bundles/keeper" - - "cosmossdk.io/math" - - globalkeeper "github.com/KYVENetwork/chain/x/global/keeper" - - "github.com/KYVENetwork/chain/x/stakers/types" - bankkeeper "github.com/cosmos/cosmos-sdk/x/bank/keeper" - - delegationkeeper "github.com/KYVENetwork/chain/x/delegation/keeper" - globalTypes "github.com/KYVENetwork/chain/x/global/types" - stakerskeeper "github.com/KYVENetwork/chain/x/stakers/keeper" - stakingkeeper "github.com/cosmos/cosmos-sdk/x/staking/keeper" - stakingTypes "github.com/cosmos/cosmos-sdk/x/staking/types" - - "cosmossdk.io/log" - upgradetypes "cosmossdk.io/x/upgrade/types" - - sdk "github.com/cosmos/cosmos-sdk/types" - "github.com/cosmos/cosmos-sdk/types/module" + + "cosmossdk.io/log" + "cosmossdk.io/math" + upgradetypes "cosmossdk.io/x/upgrade/types" + + sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/cosmos/cosmos-sdk/types/module" + authkeeper "github.com/cosmos/cosmos-sdk/x/auth/keeper" + authTypes "github.com/cosmos/cosmos-sdk/x/auth/types" + bankkeeper "github.com/cosmos/cosmos-sdk/x/bank/keeper" + stakingkeeper "github.com/cosmos/cosmos-sdk/x/staking/keeper" + stakingTypes "github.com/cosmos/cosmos-sdk/x/staking/types" + + bundleskeeper "github.com/KYVENetwork/chain/x/bundles/keeper" + delegationkeeper "github.com/KYVENetwork/chain/x/delegation/keeper" + globalkeeper "github.com/KYVENetwork/chain/x/global/keeper" + globalTypes "github.com/KYVENetwork/chain/x/global/types" + multicoinrewardskeeper "github.com/KYVENetwork/chain/x/multi_coin_rewards/keeper" + multicoinrewardstypes "github.com/KYVENetwork/chain/x/multi_coin_rewards/types" + poolkeeper "github.com/KYVENetwork/chain/x/pool/keeper" + poolTypes "github.com/KYVENetwork/chain/x/pool/types" + stakerskeeper "github.com/KYVENetwork/chain/x/stakers/keeper" + "github.com/KYVENetwork/chain/x/stakers/types" )
78-78: Address the TODO comment about setting withdraw address.The TODO comment lacks context about what needs to be implemented. Please provide more details about the withdraw address that needs to be set.
Would you like me to help implement the withdraw address setting functionality or create an issue to track this task?
91-109: Consider using constants for hardcoded values.Move the hardcoded values (addresses and time durations) to constants at the package level for better maintainability and reusability.
const ( UpgradeName = "v2.0.0" + + // Distribution pending times + MainnetDistributionPendingTime = 60 * 60 * 24 * 14 // 14 days + KaonDistributionPendingTime = 60 * 60 * 24 // 1 day + KorelliaDistributionPendingTime = 600 // 10 minutes + + // Distribution policy admin addresses + MainnetPolicyAdminAddress = "kyve1t0uez3nn28ljnzlwndzxffyjuhean3edhtjee8" + KaonPolicyAdminAddress = "kyve1z67jal9d9unjvmzsadps9jytzt9kx2m2vgc3wm" + KorelliaPolicyAdminAddress = "kyve1ygtqlzxhvp3t0wwcjd5lmq4zxl0qcck9g3mmgp" )
111-175: Add documentation for IBC denoms and pool IDs.The IBC denoms and pool IDs are hardcoded without any documentation explaining their significance. Consider adding comments to explain what each IBC denom represents and why specific pool IDs are chosen.
func SetMultiCoinRewardsPolicy(ctx sdk.Context, multiCoinRewardsKeeper multicoinrewardskeeper.Keeper) error { if ctx.ChainID() == "kyve-1" { return multiCoinRewardsKeeper.MultiCoinDistributionPolicy.Set(ctx, multicoinrewardstypes.MultiCoinDistributionPolicy{ Entries: []*multicoinrewardstypes.MultiCoinDistributionDenomEntry{ { - Denom: "ibc/A59C9E368C043E72968615DE82D4AD4BC88E34E6F353262B6769781C07390E8A", // andromeda + // Andromeda (ANDR) - IBC denom from Andromeda Hub + Denom: "ibc/A59C9E368C043E72968615DE82D4AD4BC88E34E6F353262B6769781C07390E8A", PoolWeights: []*multicoinrewardstypes.MultiCoinDistributionPoolWeightEntry{ { + // Pool 14: Andromeda mainnet archive PoolId: 14, Weight: math.LegacyMustNewDecFromStr("1"), },
180-180: Address the TODO comment about mainnet inflation split.The TODO comment lacks context about what needs to be implemented. Please provide more details about the mainnet inflation split that needs to be set.
Would you like me to help implement the mainnet inflation split functionality or create an issue to track this task?
235-304: Consider improving error handling.Instead of using
panicfor error handling, consider propagating errors up the call stack for better error management.- if err != nil { - panic(err) - } + if err != nil { + return fmt.Errorf("failed to delegate: %w", err) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/upgrades/v2_0/upgrade.go(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (3)
app/upgrades/v2_0/upgrade.go (3)
185-217: LGTM! Gas configuration looks comprehensive.The gas configuration includes all necessary message types, including the previously missing
ClaimUploaderRole. The gas prices, adjustments, and refunds are well-defined.
219-233: LGTM! Account management is robust.The function correctly handles both the creation of new module accounts and the conversion of existing accounts to module accounts.
306-366:❓ Verification inconclusive
Add version and URL validation.
Consider adding validation for:
- Version format (semver)
- Binary URLs (HTTPS, valid domains)
- Duration limits (min/max)
This will help prevent invalid or malicious upgrades.
🏁 Script executed:
#!/bin/bash # Description: Verify binary URLs are accessible and use HTTPS for url in $(rg -o 'https://[^"]*' | sort -u); do if ! curl --output /dev/null --silent --head --fail "$url"; then echo "Error: $url is not accessible" exit 1 fi doneLength of output: 481
Action: Enhance Validation in UpgradeRuntimes
Please add explicit validations to ensure that:
- Version Strings: Confirm they follow a proper semver format.
- Binary URLs: Validate that these URLs use HTTPS, point only to trusted domains, and are accessible.
- Duration Values: Ensure the upgrade duration falls within acceptable limits.
Note: The automated URL check produced a false-positive (from a non-related file:
ast-grep-rules/ast-grep-essentials/CODE_OF_CONDUCT.md), so please manually verify that the binary URLs in this upgrade code (file:app/upgrades/v2_0/upgrade.go, lines 306–366) are valid and reachable.
Summary by CodeRabbit
New Features
Chores