add membrane claims to xcclookup; fixes #1900 #1930
Conversation
WalkthroughThis update introduces support for Membrane protocol claims in the xcclookup service. It adds new data structures, claim processing logic, configuration fields, cache management, service interfaces, and test coverage to handle Membrane-related claims and assets, integrating them alongside existing protocols such as Osmosis and Umee. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AssetsService
participant CacheManager
participant ClaimsService
participant MembraneContract
User->>AssetsService: GetAssets(address)
AssetsService->>CacheManager: GetMembraneParams()
CacheManager-->>AssetsService: MembraneProtocolData
AssetsService->>ClaimsService: MembraneClaim(ctx, address, submitAddress, chain, height)
ClaimsService->>MembraneContract: Query positions for address
MembraneContract-->>ClaimsService: Positions data
ClaimsService-->>AssetsService: Claim messages, assets
AssetsService-->>User: Aggregated assets and claim results
Suggested reviewers
Poem
✨ Finishing Touches
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: 2
🧹 Nitpick comments (2)
xcclookup/pkgs/services/assets_service.go (1)
277-279: Consider using unique error keys for concurrent operations.When processing both original and mapped addresses concurrently, using the same error key "MembraneClaim" could result in one error overwriting another. Consider using unique keys like "MembraneClaim:original" and "MembraneClaim:mapped" to preserve both errors.
Also applies to: 293-295
xcclookup/pkgs/claims/membrane.go (1)
70-105: Consider returning an error when membrane params are not found.The function returns
nil, nil, nilwhen membrane params are not found (line 104), which could mask configuration issues. Consider returning an error or at least logging at a higher level thanWarn.if !found { - log.Warn("No membrane params found in cache") - return nil, nil, nil + return nil, nil, fmt.Errorf("no membrane params found in cache") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
xcclookup/config.yaml(1 hunks)xcclookup/pkgs/claims/membrane.go(1 hunks)xcclookup/pkgs/mocks/mock_cache_manager.go(2 hunks)xcclookup/pkgs/mocks/mock_claims_service.go(2 hunks)xcclookup/pkgs/services/assets_service.go(7 hunks)xcclookup/pkgs/services/assets_service_test.go(7 hunks)xcclookup/pkgs/services/claims_service.go(1 hunks)xcclookup/pkgs/types/cache.go(3 hunks)xcclookup/pkgs/types/config.go(1 hunks)xcclookup/pkgs/types/interfaces.go(2 hunks)xcclookup/pkgs/types/rpc.go(3 hunks)xcclookup/xcc.go(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
xcclookup/config.yaml (5)
Learnt from: joe-bowman
PR: quicksilver-zone/quicksilver#1894
File: third-party-chains/gaia-types/liquid/types/params.go:13-16
Timestamp: 2025-06-23T13:33:14.342Z
Learning: When reviewing code that imports types from upstream blockchain projects (like Gaia/Cosmos Hub types), do not suggest modifications to their default parameter values or behaviors, as these should remain consistent with the upstream chain's specifications.
Learnt from: joe-bowman
PR: quicksilver-zone/quicksilver#1152
File: third-party-chains/osmosis-types/gamm/pool-models/balancer/amm.go:59-59
Timestamp: 2024-10-15T13:03:53.675Z
Learning: All files within the `third-party-chains` directory are vendored files from another project, intended to provide types for cross-chain queries. Modifications to these files should be minimal to keep them as close to the original as possible.
Learnt from: joe-bowman
PR: quicksilver-zone/quicksilver#1152
File: third-party-chains/osmosis-types/gamm/pool-models/balancer/amm.go:59-59
Timestamp: 2024-07-29T07:24:15.377Z
Learning: All files within the `third-party-chains` directory are vendored files from another project, intended to provide types for cross-chain queries. Modifications to these files should be minimal to keep them as close to the original as possible.
Learnt from: joe-bowman
PR: quicksilver-zone/quicksilver#1152
File: third-party-chains/osmosis-types/gamm/pool-models/balancer/msgs.go:40-40
Timestamp: 2024-10-15T13:03:53.675Z
Learning: All files within the `third-party-chains` directory are vendored files from another project, intended to provide types for cross-chain queries. These files should be kept as close to the original as possible, and suggestions for modifications should be limited to critical functionality or integration issues.
Learnt from: joe-bowman
PR: quicksilver-zone/quicksilver#1152
File: third-party-chains/osmosis-types/gamm/pool-models/balancer/msgs.go:40-40
Timestamp: 2024-07-29T07:24:15.377Z
Learning: All files within the `third-party-chains` directory are vendored files from another project, intended to provide types for cross-chain queries. These files should be kept as close to the original as possible, and suggestions for modifications should be limited to critical functionality or integration issues.
xcclookup/pkgs/services/assets_service_test.go (1)
Learnt from: arhamchordia
PR: quicksilver-zone/quicksilver#1803
File: x/supply/keeper/keeper_test.go:31-66
Timestamp: 2025-01-28T19:03:57.668Z
Learning: Avoid suggesting test cases that duplicate functionality already tested in msg_server_test.go, particularly for module account operations and balance transfers. Focus on unique keeper functionality instead.
🧬 Code Graph Analysis (6)
xcclookup/pkgs/types/config.go (2)
x/participationrewards/types/submodule_osmosis.go (1)
OsmosisPoolProtocolData(25-33)x/participationrewards/types/protocol_data.go (1)
ConnectionProtocolData(67-73)
xcclookup/pkgs/types/interfaces.go (1)
xcclookup/pkgs/claims/membrane.go (1)
MembraneClaim(59-250)
xcclookup/pkgs/services/claims_service.go (1)
xcclookup/pkgs/claims/membrane.go (1)
MembraneClaim(59-250)
xcclookup/pkgs/services/assets_service.go (2)
xcclookup/pkgs/types/rpc.go (1)
Response(59-63)xcclookup/pkgs/claims/membrane.go (1)
MembraneClaim(59-250)
xcclookup/pkgs/services/assets_service_test.go (1)
x/participationrewards/types/protocol_data.go (1)
ConnectionProtocolData(67-73)
xcclookup/pkgs/types/cache.go (2)
x/participationrewards/types/protocol_data.go (1)
ConnectionProtocolData(67-73)x/participationrewards/types/submodule_osmosis.go (1)
OsmosisPoolProtocolData(25-33)
⏰ 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). (10)
- GitHub Check: test (1.24.5, ubuntu-latest)
- GitHub Check: build (1.24.5, ubuntu-latest)
- GitHub Check: build (1.24.5, macos-latest)
- GitHub Check: test (1.24.5, macos-latest)
- GitHub Check: Analyze
- GitHub Check: build quicksilver (amd64, darwin)
- GitHub Check: test quicksilver (darwin, arm64)
- GitHub Check: test quicksilver (amd64, linux)
- GitHub Check: test quicksilver (amd64, windows)
- GitHub Check: lint
🔇 Additional comments (27)
xcclookup/pkgs/types/config.go (1)
69-69: LGTM! Clean integration following existing patterns.The addition of
MembraneParamsfield follows the established pattern for protocol data configuration and maintains consistency with other fields in theMocksstruct.xcclookup/xcc.go (2)
131-135: LGTM! Consistent cache initialization pattern.The membrane params cache initialization follows the established pattern with appropriate error handling and 24-hour expiration, matching other protocol parameter caches.
167-167: LGTM! Consistent mock data integration.The mock data addition for MembraneParams follows the same pattern as other protocol data types.
xcclookup/config.yaml (2)
23-24: LGTM! Chain endpoint additions follow established pattern.The new chain endpoints for phoenix-1 and centauri-1 follow the consistent URL pattern used for other chains.
27-34: LGTM! Well-structured mock configuration.The membrane_params and connections mock configurations are properly structured with appropriate fields. The contract address format appears valid for the Osmosis chain.
xcclookup/pkgs/mocks/mock_cache_manager.go (2)
20-20: LGTM! Consistent mock field addition.The
GetMembraneParamsFuncfield follows the established pattern for mock function fields in the struct.
73-79: LGTM! Consistent mock method implementation.The
GetMembraneParamsmethod follows the same pattern as other mock methods with proper fallback behavior when the mock function is not set.xcclookup/pkgs/services/claims_service.go (1)
60-66: LGTM! Consistent claim method implementation.The
MembraneClaimmethod follows the established pattern used by other claim methods in the service, with proper type casting and error handling. The implementation correctly delegates to the claims package function.xcclookup/pkgs/mocks/mock_claims_service.go (2)
14-18: LGTM! Consistent mock implementation.The
MembraneClaimFuncfield follows the established pattern and has the correct signature matching other claim functions.
45-51: Well-implemented mock method.The
MembraneClaimmethod correctly implements the mock pattern, returning appropriate empty values when the mock function is not set.xcclookup/pkgs/types/interfaces.go (2)
31-31: Clean interface extension for cache manager.The
GetMembraneParamsmethod follows the established pattern for protocol data retrieval methods.
67-67: Consistent claims service interface extension.The
MembraneClaimmethod signature matches the pattern used byUmeeClaim, ensuring consistency across claim types.xcclookup/pkgs/types/rpc.go (2)
65-66: Good addition of context-aware structured logging.The context parameter enables proper structured logging throughout the method. This improves observability and debugging capabilities.
73-73: Appropriate use of log levels.Error conditions are correctly logged at error level while normal operations use debug level. The structured format with key-value pairs provides excellent context.
Also applies to: 76-76, 83-83, 86-86
xcclookup/pkgs/services/assets_service.go (3)
73-74: Membrane claims processing correctly integrated.The new membrane claims processing is added in the appropriate sequence with other claim types.
232-302: Well-structured membrane claims processor with proper error handling.The implementation correctly:
- Validates both membrane and osmosis parameters
- Handles concurrent processing for original and mapped addresses
- Uses proper mutex synchronization for error collection
Note: The dependency on Osmosis chain ID (line 270) appears intentional for cross-chain membrane claims.
133-133: Context parameter correctly added to all Update calls.All
response.Updatecalls have been properly updated to include the context parameter, maintaining consistency with the new method signature.Also applies to: 136-136, 166-166, 169-169, 211-211, 226-226, 283-283, 298-298, 325-325, 339-339
xcclookup/pkgs/types/cache.go (3)
35-41: Excellent type safety improvement with SupportedCacheTypes constraint.The type constraint centralizes all cacheable types and provides compile-time type safety for the generic cache operations. This is a clean approach that makes it easy to add new types in the future.
47-47: Generic functions properly constrained.The
GetCacheandAddMocksfunctions now benefit from compile-time type checking, preventing accidental misuse with unsupported types.Also applies to: 58-58
108-111: GetMembraneParams correctly implements the interface.The method follows the established pattern and leverages the generic
GetCachefunction appropriately.xcclookup/pkgs/services/assets_service_test.go (4)
34-46: LGTM! Test struct properly extended for membrane support.The new fields follow the established pattern and are correctly typed to support membrane claims testing.
62-64: LGTM! Test data properly configured for membrane claims.The mock data follows the established pattern with appropriate claim type and test assets.
Also applies to: 96-104
139-144: LGTM! Mock functions properly implemented.The mock implementations follow the established pattern with proper error handling.
Also applies to: 158-160
249-253: LGTM! Membrane support properly added to mapped addresses test.The mock implementations maintain consistency across test scenarios.
Also applies to: 303-312
xcclookup/pkgs/claims/membrane.go (3)
1-23: LGTM! Imports are well-organized and appropriate.All necessary dependencies are properly imported for membrane claims functionality.
25-54: LGTM! Data structures are well-designed.The structures properly model membrane positions with appropriate types and JSON tags.
167-181: LGTM! CW namespaced key construction is correct.The key construction follows the standard CosmWasm state query format with proper documentation.
xcclookup/pkgs/claims/membrane.go
Outdated
| // Add to assets | ||
| coin := sdk.NewCoin(liquidToken.QAssetDenom, collateralAsset.Asset.Amount) | ||
| assets[chain] = assets[chain].Add(coin) |
There was a problem hiding this comment.
Fix asset accumulation to use zone chain ID instead of source chain.
The assets are being accumulated under the source chain key, but they should be accumulated per zone chain ID to match the claim messages structure.
// Add to assets
coin := sdk.NewCoin(liquidToken.QAssetDenom, collateralAsset.Asset.Amount)
-assets[chain] = assets[chain].Add(coin)
+assets[zone.ChainId] = assets[zone.ChainId].Add(coin)📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Add to assets | |
| coin := sdk.NewCoin(liquidToken.QAssetDenom, collateralAsset.Asset.Amount) | |
| assets[chain] = assets[chain].Add(coin) | |
| // Add to assets | |
| coin := sdk.NewCoin(liquidToken.QAssetDenom, collateralAsset.Asset.Amount) | |
| assets[zone.ChainId] = assets[zone.ChainId].Add(coin) |
🤖 Prompt for AI Agents
In xcclookup/pkgs/claims/membrane.go around lines 234 to 236, the assets map is
currently keyed by the source chain ID, but it should use the zone chain ID to
correctly accumulate assets per zone. Update the key used in the assets map from
the source chain variable to the zone chain ID variable to align with the claim
messages structure.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1930 +/- ##
=======================================
Coverage 57.70% 57.70%
=======================================
Files 171 171
Lines 15251 15251
=======================================
Hits 8800 8800
Misses 5685 5685
Partials 766 766
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
…ed by the ClaimsService now
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
xcclookup/pkgs/claims/membrane.go (1)
221-221: Asset accumulation uses wrong chain key - confirmed issue.The assets are being accumulated under the source
chainkey, but based on the claim messages structure (lines 196-204), they should be accumulated per zone chain ID to maintain consistency.This matches the previously identified issue. The fix should be:
-assets[chain] = assets[chain].Add(coin) +assets[zone.ChainId] = assets[zone.ChainId].Add(coin)
🧹 Nitpick comments (2)
xcclookup/pkgs/claims/membrane.go (2)
91-105: Simplistic membrane params lookup needs improvement.The current implementation assumes only one membrane params entry and uses a simple break after finding the first one. This approach lacks specificity and may cause issues if multiple membrane configurations are supported in the future.
Consider adding chain-specific matching logic:
// Find the membrane params for the chain var membraneParams prewards.MembraneProtocolData found := false for _, params := range membraneParamsCache { - // For now, we assume there's only one membrane params entry - // In the future, we might need to match by chain ID + // Match by chain ID if available, otherwise use first entry for backward compatibility + if params.ChainID == chain || len(membraneParamsCache) == 1 { membraneParams = params found = true break + } }
186-230: Nested loops could benefit from performance optimization.The triple nested loop structure (positions → collateral assets → liquid tokens → zones) could be optimized by creating lookup maps for liquid tokens and zones indexed by their relevant keys.
Consider optimizing the lookup logic:
// Pre-build lookup maps outside the loops liquidTokensByDenom := make(map[string]prewards.LiquidAllowedDenomProtocolData) for _, liquidToken := range liquidAllowedDenomsCache { if liquidToken.ChainID == chain { liquidTokensByDenom[liquidToken.IbcDenom] = liquidToken } } zonesByChainId := make(map[string]icstypes.Zone) for _, zone := range zoneCache { zonesByChainId[zone.ChainId] = zone } // Then use direct lookups instead of nested loops for _, position := range positions { for _, collateralAsset := range position.CollateralAssets { if liquidToken, exists := liquidTokensByDenom[collateralAsset.Asset.Info.NativeToken.Denom]; exists { if zone, zoneExists := zonesByChainId[liquidToken.RegisteredZoneChainID]; zoneExists { // Process the claim... } } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
xcclookup/pkgs/claims/membrane.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
xcclookup/pkgs/claims/membrane.go (10)
Learnt from: joe-bowman
PR: quicksilver-zone/quicksilver#1152
File: third-party-chains/osmosis-types/gamm/pool-models/balancer/msgs.go:40-40
Timestamp: 2024-10-15T13:03:53.675Z
Learning: All files within the `third-party-chains` directory are vendored files from another project, intended to provide types for cross-chain queries. These files should be kept as close to the original as possible, and suggestions for modifications should be limited to critical functionality or integration issues.
Learnt from: joe-bowman
PR: quicksilver-zone/quicksilver#1152
File: third-party-chains/osmosis-types/gamm/pool-models/balancer/msgs.go:40-40
Timestamp: 2024-07-29T07:24:15.377Z
Learning: All files within the `third-party-chains` directory are vendored files from another project, intended to provide types for cross-chain queries. These files should be kept as close to the original as possible, and suggestions for modifications should be limited to critical functionality or integration issues.
Learnt from: joe-bowman
PR: quicksilver-zone/quicksilver#1152
File: third-party-chains/osmosis-types/gamm/pool-models/stableswap/pool.go:89-97
Timestamp: 2024-07-29T07:24:15.377Z
Learning: All files within the `third-party-chains` directory are vendored from another project and should be kept as close to the original as possible to facilitate cross-chain queries.
Learnt from: joe-bowman
PR: quicksilver-zone/quicksilver#1152
File: third-party-chains/osmosis-types/gamm/pool-models/stableswap/pool.go:89-97
Timestamp: 2024-10-15T13:03:53.675Z
Learning: All files within the `third-party-chains` directory are vendored from another project and should be kept as close to the original as possible to facilitate cross-chain queries.
Learnt from: arhamchordia
PR: quicksilver-zone/quicksilver#1803
File: x/supply/keeper/keeper_test.go:31-66
Timestamp: 2025-01-28T19:03:57.668Z
Learning: Avoid suggesting test cases that duplicate functionality already tested in msg_server_test.go, particularly for module account operations and balance transfers. Focus on unique keeper functionality instead.
Learnt from: joe-bowman
PR: quicksilver-zone/quicksilver#1152
File: third-party-chains/osmosis-types/concentrated-liquidity/errors.go:24-932
Timestamp: 2024-07-29T07:24:15.377Z
Learning: All files within the `third-party-chains` directory are vendored files from another project, intended to provide types for cross-chain queries. These files should remain as close to the original as possible.
Learnt from: joe-bowman
PR: quicksilver-zone/quicksilver#1152
File: third-party-chains/osmosis-types/gamm/pool-models/stableswap/pool.go:4-73
Timestamp: 2024-07-29T07:24:15.377Z
Learning: All files within the `third-party-chains` directory are vendored files from another project, intended to provide types for cross-chain queries. These files should remain as close to the original as possible.
Learnt from: joe-bowman
PR: quicksilver-zone/quicksilver#1152
File: third-party-chains/osmosis-types/gamm/pool-models/stableswap/pool.go:4-73
Timestamp: 2024-10-15T13:03:53.675Z
Learning: All files within the `third-party-chains` directory are vendored files from another project, intended to provide types for cross-chain queries. These files should remain as close to the original as possible.
Learnt from: joe-bowman
PR: quicksilver-zone/quicksilver#1152
File: third-party-chains/osmosis-types/gamm/pool-models/balancer/amm.go:238-238
Timestamp: 2024-07-29T07:24:15.377Z
Learning: All files within the `third-party-chains` directory are vendored files from another project, intended to provide types for cross-chain queries. These files should remain as close to the original as possible.
Learnt from: joe-bowman
PR: quicksilver-zone/quicksilver#1152
File: third-party-chains/osmosis-types/gamm/pool-models/balancer/amm.go:238-238
Timestamp: 2024-10-15T13:03:53.675Z
Learning: All files within the `third-party-chains` directory are vendored files from another project, intended to provide types for cross-chain queries. These files should remain as close to the original as possible.
⏰ 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). (5)
- GitHub Check: lint
- GitHub Check: Analyze
- GitHub Check: test quicksilver (darwin, arm64)
- GitHub Check: test quicksilver (amd64, windows)
- GitHub Check: test quicksilver (amd64, linux)
🔇 Additional comments (7)
xcclookup/pkgs/claims/membrane.go (7)
25-54: Well-structured data types for Membrane protocol integration.The struct definitions properly model the Membrane protocol's position data with appropriate field types and JSON tags. The nested structure follows Go conventions and correctly represents the hierarchical relationship between positions, collateral assets, and native tokens.
59-89: Comprehensive cache retrieval with proper error handling.The function correctly retrieves all necessary cached data with appropriate error handling and context propagation. The cache retrieval pattern is consistent and follows the established patterns in the codebase.
107-119: Osmosis params validation logic is correct.The loop correctly searches for osmosis parameters matching the specified chain and properly handles the case where no matching parameters are found.
138-147: Address conversion logic is sound.The address conversion from the input format to Osmosis bech32 format is implemented correctly with proper error handling.
151-166: CW namespaced key construction follows CosmWasm standards.The key construction properly implements the CosmWasm storage key format with the correct prefix, contract address, null terminator, and namespace structure.
167-184: ABCI query implementation with proper error handling.The ABCI query correctly uses the constructed key with appropriate options for height and proof requirements. Error handling and JSON unmarshaling are implemented properly.
206-217: Proof construction and message updating is correct.The proof construction properly extracts all necessary data from the ABCI query response, and the message updating logic correctly appends proofs to existing claims.
1. Summary
Fixes #1900
Add Membrane claims to the xcclookup daemon.
2.Type of change
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores