Arham/auto register host zone #1784
Conversation
arhamchordia
requested review from
joe-bowman,
faddat and
ajansari95
as code owners
|
Warning Rate limit exceeded@arhamchordia has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 4 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 selected for processing (2)
WalkthroughThe pull request introduces enhancements to the participation rewards module's zone creation and protocol data management. The changes focus on improving error handling and automatically registering IBC denominations for the host zone and Osmosis during zone registration. The modifications span multiple files in the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 1
🧹 Nitpick comments (5)
x/participationrewards/keeper/hooks.go (5)
5-6: Imports not GCI-compliant.
The pipeline indicates these lines are not ordered per the gci import rules. Please reorder them according to the specified format.🧰 Tools
🪛 golangci-lint (1.62.2)
6-6: File is not
gci-ed with --skip-generated -s standard -s default -s blank -s dot -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) -s prefix(github.com/cometbft/cometbft) -s prefix(github.com/cosmos/ibc-go) -s prefix(github.com/quicksilver-zone/quicksilver) --custom-order(gci)
9-10: Imports not GCI-compliant.
These lines also violate the gci import ordering rules. Please reorder as indicated by the pipeline.🧰 Tools
🪛 golangci-lint (1.62.2)
10-10: File is not
gci-ed with --skip-generated -s standard -s default -s blank -s dot -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) -s prefix(github.com/cometbft/cometbft) -s prefix(github.com/cosmos/ibc-go) -s prefix(github.com/quicksilver-zone/quicksilver) --custom-order(gci)
152-157: Consider encapsulating repeated "GetChannel" checks into a helper.
This logic of retrieving the channel and returning an error if not found appears multiple times (also in Osmosis and Umee sections). Factoring this out to a dedicated helper function can reduce duplication and improve readability.
177-218: Duplicated code for Osmosis chain.
This block introduces almost identical logic as the host zone block. Consider extracting a shared utility function that handles chain-specific channel lookups and LiquidAllowedDenomProtocolData creation to maintain DRY principles.
220-262: Repeated pattern for Umee chain.
The same pattern of protocol data retrieval, channel verification, and LiquidAllowedDenomProtocolData creation is repeated. A shared helper for all chains would simplify future maintenance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
x/participationrewards/keeper/hooks.go(2 hunks)x/participationrewards/keeper/hooks_test.go(3 hunks)x/participationrewards/keeper/keeper_test.go(5 hunks)x/participationrewards/keeper/proposal_handler_test.go(1 hunks)x/participationrewards/keeper/protocol_data.go(2 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
x/participationrewards/keeper/hooks.go
6-6: File is not gci-ed with --skip-generated -s standard -s default -s blank -s dot -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) -s prefix(github.com/cometbft/cometbft) -s prefix(github.com/cosmos/ibc-go) -s prefix(github.com/quicksilver-zone/quicksilver) --custom-order
(gci)
8-8: File is not gci-ed with --skip-generated -s standard -s default -s blank -s dot -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) -s prefix(github.com/cometbft/cometbft) -s prefix(github.com/cosmos/ibc-go) -s prefix(github.com/quicksilver-zone/quicksilver) --custom-order
(gci)
10-10: File is not gci-ed with --skip-generated -s standard -s default -s blank -s dot -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) -s prefix(github.com/cometbft/cometbft) -s prefix(github.com/cosmos/ibc-go) -s prefix(github.com/quicksilver-zone/quicksilver) --custom-order
(gci)
x/participationrewards/keeper/keeper_test.go
39-39: var osmosisBaseDenom is unused
(unused)
🪛 GitHub Actions: golangci-lint
x/participationrewards/keeper/hooks.go
[warning] 6-11: File is not properly formatted with 'gci'. Imports need to be ordered according to the specified format: standard, default, blank, dot, and prefix-based ordering.
x/participationrewards/keeper/keeper_test.go
[error] 39-39: Unused variable 'osmosisBaseDenom'
🪛 GitHub Check: lint
x/participationrewards/keeper/keeper_test.go
[failure] 39-39:
var osmosisBaseDenom is unused (unused)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test quicksilver (darwin, arm64)
- GitHub Check: test quicksilver (amd64, windows)
- GitHub Check: test quicksilver (amd64, linux)
- GitHub Check: Analyze
🔇 Additional comments (19)
x/participationrewards/keeper/hooks.go (1)
158-176: Logic for host zone LiquidAllowedDenomProtocolData looks good.
The creation and validation ofhostZoneDenomis correct and straightforward.x/participationrewards/keeper/hooks_test.go (5)
4-4: Import addition approved.
Importing"github.com/quicksilver-zone/quicksilver/utils"is appropriate for the new test logic.
19-20: Improved testing setup.
CallingsetupChannelForHookTest()andsetupTestProtocolData()ensures the environment is fully initialized. This is a beneficial enhancement for test clarity.
36-47: Host chain IBC denom check is comprehensive.
Verifying the correct IBC denom for the host chain improves coverage and confidence in the new logic.
48-58: Osmosis IBC denom checks.
These checks mirror the host chain verification and maintain consistency for multi-chain scenarios.
59-68: Umee IBC denom checks.
Likewise, verifying the IBC denom for Umee ensures correctness across chains.x/participationrewards/keeper/proposal_handler_test.go (6)
113-114: Context initialization is standard.
Creating a local reference to the context and keeper is typical for proposal tests. No issues found.
116-116: Preparing protocol data struct.
Defining aConnectionProtocolDatainstance is clear and straightforward.
123-123: Utilizing MarshalAndSetProtocolData.
Good job using the new helper function. This improves consistency in storing protocol data.
126-126: Verifying protocol data existence.
Ensuring the data is actually stored before removal is a sensible check for the test scenario.
132-136: Proposal message for removal.
Constructing the removal proposal with a base64-encoded key is aligned with the updated approach.
144-144: Asserting protocol data removal.
Confirming that the data is no longer found after the removal proposal is processed completes the test coverage.x/participationrewards/keeper/protocol_data.go (1)
111-123: New helper function improves consistency.
MarshalAndSetProtocolDatacentralizes the marshaling and storing of protocol data, reducing repeated boilerplate code. Great addition!x/participationrewards/keeper/keeper_test.go (6)
45-49: Channel identifiers for multiple test chains.
Defining these constants clarifies the test environment configuration for ICS.
118-118: Updated expected protocol data count.
Changing from 14 to 15 aligns with the newly added data sets.
417-431: Channel setup function is beneficial.
Creating multiple channels in a single helper prepares the environment for testing complex multi-zone logic.
432-432: Extra blank line.
No functional changes.
Line range hint
434-452: Additional protocol data setup.
Extended protocol data for new test scenarios is consistent with the changes in the hooks logic.
492-492: New connection data for Osmosis chain.
Aligns with the added test coverage for chain-2.
| @@ -35,10 +35,19 @@ | |||
| umeeTestChain = "umee-types-1" | |||
| umeeBaseDenom = "uumee" | |||
|
|
|||
| osmosisTestChain = "osmosis-1" | |||
| osmosisBaseDenom = "uosmo" | |||
There was a problem hiding this comment.
Unused variable triggers pipeline error.
osmosisBaseDenom is reported as unused and fails the lint check. Consider removing it if it's not needed.
Apply this diff to remove the variable:
- osmosisBaseDenom = "uosmo"🧰 Tools
🪛 golangci-lint (1.62.2)
39-39: var osmosisBaseDenom is unused
(unused)
🪛 GitHub Check: lint
[failure] 39-39:
var osmosisBaseDenom is unused (unused)
🪛 GitHub Actions: golangci-lint
[error] 39-39: Unused variable 'osmosisBaseDenom'
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1784 +/- ##
==========================================
- Coverage 61.43% 61.38% -0.05%
==========================================
Files 196 196
Lines 17117 17228 +111
==========================================
+ Hits 10515 10575 +60
- Misses 5748 5783 +35
- Partials 854 870 +16
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
@arhamchordia - can you run |
1. Summary
Fixes #1458
2.Type of change
3. Implementation details
4. How to test/use
5. Checklist
6. Limitations (optional)
7. Future Work (optional)
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Chores