Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ensure we use the correct zone denom where evaluating pool claims #1721

Merged
merged 1 commit into from
Sep 26, 2024
Merged

ensure we use the correct zone denom where evaluating pool claims #1721

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 8 additions & 28 deletions third-party-chains/osmosis-types/validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"

osmosislockuptypes "github.com/quicksilver-zone/quicksilver/third-party-chains/osmosis-types/lockup"
"github.com/quicksilver-zone/quicksilver/utils"
participationrewardstypes "github.com/quicksilver-zone/quicksilver/x/participationrewards/types"

cl "github.com/quicksilver-zone/quicksilver/third-party-chains/osmosis-types/concentrated-liquidity"
Expand All @@ -22,7 +21,7 @@ type ParticipationRewardsKeeper interface {
GetProtocolData(ctx sdk.Context, pdType participationrewardstypes.ProtocolDataType, key string) (participationrewardstypes.ProtocolData, bool)
}

func DetermineApplicableTokensInPool(ctx sdk.Context, prKeeper ParticipationRewardsKeeper, lock osmosislockuptypes.PeriodLock, chainID string) (math.Int, error) {
func DetermineApplicableTokensInPool(ctx sdk.Context, prKeeper ParticipationRewardsKeeper, lock osmosislockuptypes.PeriodLock, chainID string, poolDenom string) (math.Int, error) {
gammtoken, err := lock.SingleCoin()
if err != nil {
return sdk.ZeroInt(), err
Expand All @@ -40,18 +39,6 @@ func DetermineApplicableTokensInPool(ctx sdk.Context, prKeeper ParticipationRewa
}
pool, _ := ipool.(*participationrewardstypes.OsmosisPoolProtocolData)

poolDenom := ""
for _, zk := range utils.Keys(pool.Denoms) {
if pool.Denoms[zk].ChainID == chainID {
poolDenom = zk
break
}
}

if poolDenom == "" {
return sdk.ZeroInt(), fmt.Errorf("invalid zone, pool zone must match %s", chainID)
}

poolData, err := pool.GetPool()
if err != nil {
return sdk.ZeroInt(), err
Expand Down Expand Up @@ -91,8 +78,10 @@ func CalculateUnderlyingAssetsFromPosition(ctx sdk.Context, position clmodel.Pos
return coin0, coin1, nil
}

func DetermineApplicableTokensInClPool(ctx sdk.Context, prKeeper ParticipationRewardsKeeper, position clmodel.Position, chainID string) (math.Int, error) {
func DetermineApplicableTokensInClPool(ctx sdk.Context, prKeeper ParticipationRewardsKeeper, position clmodel.Position, chainID string, poolDenom string) (math.Int, error) {
poolID := position.PoolId

ctx.Logger().Info("DetermineApplicableTokensInClPool", "poolID", poolID, "position", position)
pd, ok := prKeeper.GetProtocolData(ctx, participationrewardstypes.ProtocolDataTypeOsmosisCLPool, fmt.Sprintf("%d", poolID))
if !ok {
return sdk.ZeroInt(), fmt.Errorf("unable to obtain protocol data for poolID=%d", poolID)
Expand All @@ -104,18 +93,6 @@ func DetermineApplicableTokensInClPool(ctx sdk.Context, prKeeper ParticipationRe
}
pool, _ := ipool.(*participationrewardstypes.OsmosisClPoolProtocolData)

poolDenom := ""
for _, zk := range utils.Keys(pool.Denoms) {
if pool.Denoms[zk].ChainID == chainID {
poolDenom = zk
break
}
}

if poolDenom == "" {
return sdk.ZeroInt(), fmt.Errorf("invalid zone, pool zone must match %s", chainID)
}

poolData, err := pool.GetPool()
if err != nil {
return sdk.ZeroInt(), err
Expand All @@ -126,13 +103,16 @@ func DetermineApplicableTokensInClPool(ctx sdk.Context, prKeeper ParticipationRe
if err != nil {
return sdk.ZeroInt(), errors.New("unable to determine underlying assets for position")
}

ctx.Logger().Info("DetermineApplicableTokensInClPool", "asset0", asset0, "asset1", asset1)

switch true {
case asset0.Denom == poolDenom:
asset = asset0
case asset1.Denom == poolDenom:
asset = asset1
default:
return sdk.ZeroInt(), fmt.Errorf("position does not match local denom for %s", chainID)
return sdk.ZeroInt(), fmt.Errorf("position does not match local denom for %s (poolDenom: %s)", chainID, poolDenom)
}

return asset.Amount, nil
Expand Down
2 changes: 1 addition & 1 deletion x/airdrop/keeper/claim_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@ func (k *Keeper) verifyOsmosisLP(ctx sdk.Context, proofs []*cmtypes.Proof, cr ty
}

func (k *Keeper) verifyPoolAndGetAmount(ctx sdk.Context, lock osmosislockuptypes.PeriodLock, cr types.ClaimRecord) (sdkmath.Int, error) {
return osmosistypes.DetermineApplicableTokensInPool(ctx, k.prKeeper, lock, cr.ChainId)
return osmosistypes.DetermineApplicableTokensInPool(ctx, k.prKeeper, lock, cr.ChainId, "UNUSED")
}

// -----------
Expand Down
8 changes: 7 additions & 1 deletion x/participationrewards/keeper/submodule_osmosis.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,13 @@ func (*OsmosisModule) ValidateClaim(ctx sdk.Context, k *Keeper, msg *types.MsgSu
}
}
}
sdkAmount, err := osmosistypes.DetermineApplicableTokensInPool(ctx, k, lock, msg.Zone)

denom, found := k.ApplicableDenomForZone(ctx, msg.Zone)
if !found {
return math.ZeroInt(), errors.New("no applicable denom found for zone")
}
Comment on lines +143 to +146
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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")
}


sdkAmount, err := osmosistypes.DetermineApplicableTokensInPool(ctx, k, lock, msg.Zone, denom)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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

if err != nil {
return sdk.ZeroInt(), err
}
Expand Down
36 changes: 35 additions & 1 deletion x/participationrewards/keeper/submodule_osmosiscl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,12 @@
}
}

sdkAmount, err := osmosistypes.DetermineApplicableTokensInClPool(ctx, k, position, msg.Zone)
denom, found := k.ApplicableDenomForZone(ctx, msg.Zone)
if !found {
return math.ZeroInt(), errors.New("no applicable denom found for zone")
}

sdkAmount, err := osmosistypes.DetermineApplicableTokensInClPool(ctx, k, position, msg.Zone, denom)
if err != nil {
return math.ZeroInt(), err
}
Expand All @@ -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) {

Check failure on line 135 in x/participationrewards/keeper/submodule_osmosiscl.go

View workflow job for this annotation

GitHub Actions / lint 

ST1003: method parameter chainId should be chainID (stylecheck)

Check failure on line 135 in x/participationrewards/keeper/submodule_osmosiscl.go

View workflow job for this annotation

GitHub Actions / lint 

ST1003: method parameter chainId should be chainID (stylecheck)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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)

zone, found := k.icsKeeper.GetZone(ctx, chainId)
if !found {
return "", false
}

params, found := k.GetProtocolData(ctx, types.ProtocolDataTypeOsmosisParams, types.OsmosisParamsKey)
if !found {
return "", false
}

paramsData := types.OsmosisParamsProtocolData{}
if err := json.Unmarshal(params.Data, &paramsData); err != nil {
return "", false
}

k.IteratePrefixedProtocolDatas(ctx, types.GetPrefixProtocolDataKey(types.ProtocolDataTypeLiquidToken), func(idx int64, key []byte, data types.ProtocolData) bool {
liquidToken, _ := types.UnmarshalProtocolData(types.ProtocolDataTypeLiquidToken, data.Data)
liquidTokenData := liquidToken.(*types.LiquidAllowedDenomProtocolData)
Comment on lines +152 to +153
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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
}

if liquidTokenData.ChainID == paramsData.ChainID && liquidTokenData.QAssetDenom == zone.LocalDenom {
found = true
denom = liquidTokenData.IbcDenom
return true
}
return false
})
return denom, found
}
Loading