ensure we use the correct zone denom where evaluating pool claims #1721
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe pull request introduces modifications to several functions related to token determination in pools within the Osmosis project. Key changes include the addition of a Changes
Possibly related PRs
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: 4
🧹 Outside diff range and nitpick comments (3)
third-party-chains/osmosis-types/validation.go (3)
24-24: LGTM! Consider enhancing error handling.The changes to the function signature and the use of
poolDenomalign well with the PR objectives. This modification addresses the issue of incorrect denom selection and simplifies the logic.Consider adding a check at the beginning of the function to ensure that
poolDenomis not empty:if poolDenom == "" { return sdk.ZeroInt(), errors.New("poolDenom cannot be empty") }This would provide an early exit and clearer error message if an invalid
poolDenomis passed.
Line range hint
81-119: Verify poolDenom against ProtocolDataAllowedLiquidToken list.The PR objectives mention a new requirement: "all claimable assets in a pool must exist in the ProtocolDataAllowedLiquidToken list". However, the current implementation doesn't include this check.
Consider adding a verification step to ensure that the
poolDenomexists in the ProtocolDataAllowedLiquidToken list. This could be implemented as follows:// Add this near the beginning of the function, after obtaining the protocol data osmosisParams, ok := prKeeper.GetProtocolData(ctx, participationrewardstypes.ProtocolDataTypeOsmosisParams, "") if !ok { return sdk.ZeroInt(), errors.New("unable to obtain Osmosis params protocol data") } params, err := participationrewardstypes.UnmarshalProtocolData(participationrewardstypes.ProtocolDataTypeOsmosisParams, osmosisParams.Data) if err != nil { return sdk.ZeroInt(), fmt.Errorf("failed to unmarshal Osmosis params: %w", err) } osmosisParamsData, ok := params.(*participationrewardstypes.OsmosisParamsProtocolData) if !ok { return sdk.ZeroInt(), errors.New("invalid Osmosis params data type") } if !osmosisParamsData.IsAllowedLiquidToken(poolDenom) { return sdk.ZeroInt(), fmt.Errorf("poolDenom %s is not in the ProtocolDataAllowedLiquidToken list", poolDenom) }This addition ensures that the
poolDenomis verified against the ProtocolDataAllowedLiquidToken list, as specified in the PR objectives.
Line range hint
1-119: Summary: Changes align with PR objectives, but additional improvements suggested.The modifications to both
DetermineApplicableTokensInPoolandDetermineApplicableTokensInClPoolfunctions address the core issue of incorrect denom selection. The addition of thepoolDenomparameter and its direct usage simplify the logic and reduce the potential for errors.However, there are two areas for improvement:
- Enhanced error handling in
DetermineApplicableTokensInPoolto check for emptypoolDenom.- Implementation of the requirement to verify the
poolDenomagainst the ProtocolDataAllowedLiquidToken list inDetermineApplicableTokensInClPool.Addressing these points will further strengthen the implementation and fully meet the PR objectives.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- third-party-chains/osmosis-types/validation.go (3 hunks)
- x/airdrop/keeper/claim_handler.go (1 hunks)
- x/participationrewards/keeper/submodule_osmosis.go (1 hunks)
- x/participationrewards/keeper/submodule_osmosiscl.go (2 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
x/participationrewards/keeper/submodule_osmosiscl.go
[failure] 135-135:
ST1003: method parameter chainId should be chainID (stylecheck)
🔇 Additional comments (2)
third-party-chains/osmosis-types/validation.go (1)
81-83: LGTM! Improved logging enhances debuggability.The changes to the function signature and the use of
poolDenomalign well with the PR objectives. The added logging statements at lines 84 and 107 will greatly improve debuggability. The updated error message at line 115 provides more context, which is helpful.Also applies to: 84-84, 107-108, 115-115
x/airdrop/keeper/claim_handler.go (1)
310-310: Approved, but clarification needed on the new parameter.The addition of the "UNUSED" parameter to the
DetermineApplicableTokensInPoolfunction call is a non-breaking change. However, there are a few points that need clarification:
What is the purpose of the "UNUSED" parameter? It's not clear how this relates to the PR objectives of ensuring the correct zone denom is used for evaluating pool claims.
How does this change align with the PR objective of referencing the ProtocolDataAllowedLiquidToken? The current implementation doesn't seem to directly address this.
Is this a temporary solution or a preparation for future changes?
To better understand the context and impact of this change, let's check how the
DetermineApplicableTokensInPoolfunction is defined and used elsewhere in the codebase:Consider adding a comment to explain the purpose of the "UNUSED" parameter:
func (k *Keeper) verifyPoolAndGetAmount(ctx sdk.Context, lock osmosislockuptypes.PeriodLock, cr types.ClaimRecord) (sdkmath.Int, error) { + // TODO: Explain the purpose of the "UNUSED" parameter return osmosistypes.DetermineApplicableTokensInPool(ctx, k.prKeeper, lock, cr.ChainId, "UNUSED") }
| denom, found := k.ApplicableDenomForZone(ctx, msg.Zone) | ||
| if !found { | ||
| return math.ZeroInt(), errors.New("no applicable denom found for zone") | ||
| } |
There was a problem hiding this comment.
Use sdk.ZeroInt() instead of math.ZeroInt() for error return
In line 145, you return math.ZeroInt(), whereas elsewhere in the code, sdk.ZeroInt() is used for returning zero Int values. For consistency and to avoid potential issues, consider replacing math.ZeroInt() with sdk.ZeroInt().
Apply this diff to fix the inconsistency:
- return math.ZeroInt(), errors.New("no applicable denom found for zone")
+ return sdk.ZeroInt(), errors.New("no applicable denom found for zone")📝 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.
| denom, found := k.ApplicableDenomForZone(ctx, msg.Zone) | |
| if !found { | |
| return math.ZeroInt(), errors.New("no applicable denom found for zone") | |
| } | |
| denom, found := k.ApplicableDenomForZone(ctx, msg.Zone) | |
| if !found { | |
| return sdk.ZeroInt(), errors.New("no applicable denom found for zone") | |
| } |
| return math.ZeroInt(), errors.New("no applicable denom found for zone") | ||
| } | ||
|
|
||
| sdkAmount, err := osmosistypes.DetermineApplicableTokensInPool(ctx, k, lock, msg.Zone, denom) |
There was a problem hiding this comment.
💡 Codebase verification
Incomplete Update of DetermineApplicableTokensInPool Calls
The call in x/airdrop/keeper/claim_handler.go is passing "UNUSED" as the denom parameter. This appears to be a placeholder and should be updated to use a valid denomination to prevent potential runtime errors.
🔗 Analysis chain
Ensure all calls to DetermineApplicableTokensInPool include the new denom parameter
The function DetermineApplicableTokensInPool now requires an additional denom parameter. Please verify that all other calls to this function in the codebase have been updated to include the new parameter to prevent any runtime errors.
Run the following script to find any usages of DetermineApplicableTokensInPool that may need updating:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find calls to `DetermineApplicableTokensInPool` without the `denom` parameter
# Expected: No occurrences of calls with missing parameters
rg --type go 'DetermineApplicableTokensInPool\(' -A5
Length of output: 1464
Script:
#!/bin/bash
# Description: Verify if the 'denom' parameter is intentionally set to "UNUSED" in claim_handler.go
# Expected: Identify any comments or TODOs indicating the purpose of "UNUSED"
rg --type go 'DetermineApplicableTokensInPool\(' x/airdrop/keeper/claim_handler.go -A2
Length of output: 189
Script:
#!/bin/bash
# Description: Search for definitions and usages of "UNUSED" in the codebase
# Expected: Identify if "UNUSED" is defined as a constant or used in multiple contexts
rg '"UNUSED"' --no-filename -C2
Length of output: 273
| @@ -126,3 +131,32 @@ | |||
| func (*OsmosisClModule) KeyPool(poolID uint64) []byte { | |||
| return osmocl.KeyPool(poolID) | |||
| } | |||
|
|
|||
| func (k *Keeper) ApplicableDenomForZone(ctx sdk.Context, chainId string) (denom string, found bool) { | |||
There was a problem hiding this comment.
Correct parameter name to follow Go naming conventions
According to Go naming conventions, acronyms should be capitalized. Therefore, chainId should be renamed to chainID.
Apply this diff to correct the parameter name:
-func (k *Keeper) ApplicableDenomForZone(ctx sdk.Context, chainId string) (denom string, found bool) {
+func (k *Keeper) ApplicableDenomForZone(ctx sdk.Context, chainID string) (denom string, found bool) {Also, update all instances of chainId within the function:
- zone, found := k.icsKeeper.GetZone(ctx, chainId)
+ zone, found := k.icsKeeper.GetZone(ctx, chainID)📝 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.
| func (k *Keeper) ApplicableDenomForZone(ctx sdk.Context, chainId string) (denom string, found bool) { | |
| func (k *Keeper) ApplicableDenomForZone(ctx sdk.Context, chainID string) (denom string, found bool) { |
🧰 Tools
🪛 GitHub Check: lint
[failure] 135-135:
ST1003: method parameter chainId should be chainID (stylecheck)
| liquidToken, _ := types.UnmarshalProtocolData(types.ProtocolDataTypeLiquidToken, data.Data) | ||
| liquidTokenData := liquidToken.(*types.LiquidAllowedDenomProtocolData) |
There was a problem hiding this comment.
Handle errors during unmarshalling and type assertion
The error returned by UnmarshalProtocolData is being ignored, and the unchecked type assertion could result in a runtime panic if the assertion fails. It's important to handle these errors to prevent potential runtime issues.
Consider modifying the code to handle the error and safely perform the type assertion:
- liquidToken, _ := types.UnmarshalProtocolData(types.ProtocolDataTypeLiquidToken, data.Data)
- liquidTokenData := liquidToken.(*types.LiquidAllowedDenomProtocolData)
+ liquidToken, err := types.UnmarshalProtocolData(types.ProtocolDataTypeLiquidToken, data.Data)
+ if err != nil {
+ // Handle the error appropriately, possibly log and continue
+ return false
+ }
+ liquidTokenData, ok := liquidToken.(*types.LiquidAllowedDenomProtocolData)
+ if !ok {
+ // Handle the unexpected type
+ return false
+ }📝 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.
| liquidToken, _ := types.UnmarshalProtocolData(types.ProtocolDataTypeLiquidToken, data.Data) | |
| liquidTokenData := liquidToken.(*types.LiquidAllowedDenomProtocolData) | |
| liquidToken, err := types.UnmarshalProtocolData(types.ProtocolDataTypeLiquidToken, data.Data) | |
| if err != nil { | |
| // Handle the error appropriately, possibly log and continue | |
| return false | |
| } | |
| liquidTokenData, ok := liquidToken.(*types.LiquidAllowedDenomProtocolData) | |
| if !ok { | |
| // Handle the unexpected type | |
| return false | |
| } |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1721 +/- ##
==========================================
- Coverage 63.44% 63.41% -0.03%
==========================================
Files 194 194
Lines 13411 13436 +25
==========================================
+ Hits 8508 8521 +13
- Misses 4089 4097 +8
- Partials 814 818 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
1. Summary
Osmosis Pools (legacy + CL) attempt to validate the denom for a claim against the zone, by checking the chain_id of the denom vs the chain_id of the zone. Given that the chain_id of a qAsset/asset pool will be the same for both denoms, the denom selected will be based on the order of the denoms in the slice - and not always validate the correct denom.
This change ensures we use the correct denom as listed for that qAsset on osmosis by the ProtocolDataAllowedLiquidToken.
2.Type of change
3. Implementation details
Instead of checking whether denom X has chain_id Y (to match the zone), we lookup the local_denom of the zone, cross reference against the ProtocolDataAllowedLiquidToken list for osmosis (via the OsmosisParams PD chain_id), to ascertain whether we match on the denom. This is always guaranteed to yield the correct answer.
This introduces a contract that all claimable assets in a pool MUST exist in the ProtocolDataAllowedLiquidToken list - however, I cannot foresee any instance where this isn't desirable.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor