[NonPoolSuperfluid] Queries #8320
Conversation
…ative is properly tested
…ns aren't returned after delegation ends
…ependent-risk-factor
nicolaslara
requested review from
p0mvn,
AlpinYukseloglu,
mattverse and
czarcas7ic
as code owners
github-actions
bot
added
C:x/superfluid
C:x/twap
nicolaslara
changed the base branch from
main
to
nicolas/superfluid-btc-multiple-pools
WalkthroughThis update enhances the Osmosis protocol by adding support for non-pool assets in superfluid staking. Key changes include modifying protobuf definitions to include new fields, updating various keeper functions and tests to handle the new asset types, and introducing new methods for calculating and validating non-native assets. The overall goal is to improve the flexibility and functionality of superfluid staking. Changes
Possibly related issues
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (3)
Files skipped from review as they are similar to previous changes (2)
Additional context usedMarkdownlint
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
nicolaslara
added
the
V:state/compatible/no_backport
![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
Outside diff range and nitpick comments (10)
x/superfluid/keeper/twap_price.go (1)
60-60: Clarify the logic for excluding native tokens ingetSuperfluidOSMOTokens.Consider adding a comment explaining why native tokens are excluded in some scenarios.
CHANGELOG.md (9)
Line range hint
1058-1058: Correct the heading level to maintain a logical order.- #### [v6.4.0](https://github.com/osmosis-labs/osmosis/releases/tag/v6.4.0) + ### [v6.4.0](https://github.com/osmosis-labs/osmosis/releases/tag/v6.4.0)
Line range hint
1070-1070: Correct the heading level to maintain a logical order.- #### [v6.3.1](https://github.com/osmosis-labs/osmosis/releases/tag/v6.3.1) + ### [v6.3.1](https://github.com/osmosis-labs/osmosis/releases/tag/v6.3.1)
Line range hint
1079-1079: Correct the heading level to maintain a logical order.- #### [v6.3.0](https://github.com/osmosis-labs/osmosis/releases/tag/v6.3.0) + ### [v6.3.0](https://github.com/osmosis-labs/osmosis/releases/tag/v6.3.0)
Line range hint
1084-1084: Correct the heading level to maintain a logical order.- #### [v6.2.0](https://github.com/osmosis-labs/osmosis/releases/tag/v6.2.0) + ### [v6.2.0](https://github.com/osmosis-labs/osmosis/releases/tag/v6.2.0)
Line range hint
1136-1136: Correct the heading level to maintain a logical order.- #### [v6.0.0](https://github.com/osmosis-labs/osmosis/releases/tag/v6.0.0) + ### [v6.0.0](https://github.com/osmosis-labs/osmosis/releases/tag/v6.0.0)
Line range hint
397-397: Remove trailing punctuation from the heading.- ## [v4.0.0] + ## [v4.0.0]
Line range hint
660-660: Replace bare URL with a markdown link.- See: [SDK v0.43.0 Release Notes](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.43.0) For more details + See: [SDK v0.43.0 Release Notes](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.43.0) for more details.
Line range hint
718-718: Replace bare URL with a markdown link.- [wasmd-v.022.0-osmo-v7.2](https://github.com/osmosis-labs/wasmd/releases/tag/v0.22.0-osmo-v7.2) Upgrade SDK and IAVL dependencies to use fast storage + [wasmd-v.022.0-osmo-v7.2](https://github.com/osmosis-labs/wasmd/releases/tag/v0.22.0-osmo-v7.2) Upgrade SDK and IAVL dependencies to use fast storage.
Line range hint
1299-1299: Ensure all links are properly formatted and not empty.- * [Patch](https://github.com/osmosis-labs/osmosis/commit/907001b08686ed980e0afa3d97a9c5e2f095b79f#diff-a172cedcae47474b615c54d510a5d84a8dea3032e958587430b413538be3f333) - Revert back to passing in the correct staking keeper into the IBC keeper constructor. + * [Patch](https://github.com/osmosis-labs/osmosis/commit/907001b08686ed980e0afa3d97a9c5e2f095b79f#diff-a172cedcae47474b615c54d510a5d84a8dea3032e958587430b413538be3f333) - Revert back to passing in the correct staking keeper into the IBC keeper constructor.
| @@ -430,7 +435,7 @@ func (q Querier) SuperfluidDelegationsByValidatorDenom(goCtx context.Context, re | |||
| lockedCoins := sdk.NewCoin(req.Denom, lock.GetCoins().AmountOf(req.Denom)) | |||
| baseDenom := lock.Coins.GetDenomByIndex(0) | |||
|
|
|||
| equivalentAmount, err := q.Keeper.GetSuperfluidOSMOTokensExcludeNative(ctx, baseDenom, lockedCoins.Amount) | |||
| equivalentAmount, err := q.Keeper.GetSuperfluidOSMOTokens(ctx, baseDenom, lockedCoins.Amount) | |||
There was a problem hiding this comment.
Wont this include uosmo then? I think should be excluded in SF queries, but all other native SF assets make sense.
There was a problem hiding this comment.
the idea is to include everything there. I added a separate field for the non-osmo ones. To get the old value, we can subtract them.
I thought of keeping this one as the one with only osmo and adding the field for non-osmo, but I think the query response names would become confusing: why would the field total_equivalent_staked_amount exclude some assets? I think "total" implies all.
I choose to change the value instead of the name here, but happy to change the api to make it more clear
There was a problem hiding this comment.
But this query is total SF assets delegated, not total assets delegated. So it would make sense to not include uosmo (as a native asset), since that would include uosmo staked to the validator by normal staking means.
There was a problem hiding this comment.
In other words, with no other changes and just this query changed, we would go from showing the SF assets staked to a validator, to the SF assets staked to a validator plus the normal uosmo staked, which completely changes what the query means (and imo doesn't make sense to do it this way). If we only include SF assets, then the before and after of this query is the same, and will only change once a new native SF asset gets added.
There was a problem hiding this comment.
This gets the total SF staked. Neither the one before nor after include the staked uosmo. When I say non-pool assets here, I mean btc.
If we only include SF assets, then the before and after of this query is the same, and will only change once a new native SF asset gets added.
correct
There was a problem hiding this comment.
The "exclude native" really throws me off then, why it was ever named this
There was a problem hiding this comment.
I'd be happy to rename them to something with OsmoEquivalent to make it clearer. Just didn't want to change the existing name
There was a problem hiding this comment.
The context in which the method is being called is what is confusing me.
All things equal, in SF if I see "ExcludeNative" in context of superfluid and this new feature, I think, lets get all the superfluid staked assets of a certain validator, but leave out any osmo or token factory tokens (in this case alloyed assets).
I think what is missing is a proper godoc here
osmosis/x/superfluid/keeper/twap_price.go
Lines 59 to 76 in ae8bf49
There was a problem hiding this comment.
I fully onboarded and IMO, this clarifies the difference between the two methods abd9f3b
* initial docs * more docs * Update x/superfluid/README.md * lint * clarify native vs non native calls --------- Co-authored-by: Adam Tucker <[email protected]> Co-authored-by: Adam Tucker <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 12
Outside diff range and nitpick comments (1)
x/superfluid/keeper/integration_test.go (1)
Line range hint
501-514: The test setup for reward distribution appears to have a potential issue with the setup of validator rewards. The comment on line 512 suggests a misunderstanding or a setup error in the test environment. It would be beneficial to clarify this in the test comments or revise the setup to avoid confusion.- // Validators get stake and uosmo. I think this is an issue with test setup + // Ensure that validators receive the correct types of rewards. Adjust the test setup if necessary.
| ### Set a custom Risk Factor for a denom | ||
|
|
||
| The risk factor is a multiplier that is applied to the Osmo Equivalent | ||
| Multiplier for a denom. It is used to adjust the staking power of a | ||
| denom. The risk factor is set by the governance module. | ||
|
|
||
| If not set, it defaults to params.minimum_risk_factor. | ||
|
|
||
| ```{.go} | ||
| type MsgSetRiskFactor struct { | ||
| Sender string | ||
| Denom string | ||
| RiskFactor github.com_cosmos_cosmos_sdk_types.Dec | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Clarify the default behavior for the risk factor if not set.
The documentation mentions that the risk factor defaults to params.minimum_risk_factor if not set. It would be beneficial to include this default behavior in the MsgSetRiskFactor struct documentation to ensure clarity for developers and users.
x/superfluid/keeper/epoch.go
Outdated
| price, err := k.twapk.GetMultiPoolArithmeticTwapToNow(ctx, asset.PriceRoute, asset.Denom, bondDenom, startTime) | ||
| if err != nil { | ||
| return sdkerrors.Wrap(err, "failed to get twap price") | ||
| } | ||
| k.SetOsmoEquivalentMultiplier(ctx, newEpochNumber, asset.Denom, osmomath.NewDec(1).Quo(price)) | ||
| k.SetOsmoEquivalentMultiplier(ctx, newEpochNumber, asset.Denom, price) |
There was a problem hiding this comment.
Consider handling the error from GetMultiPoolArithmeticTwapToNow more gracefully.
Currently, the function returns an error directly which could halt the execution of important logic if the TWAP calculation fails. It might be beneficial to log this error and continue with a default or previously calculated value to ensure the system's robustness.
| s.Require().Equal(appparams.BaseCoinUnit, res.SuperfluidDelegationRecords[0].EquivalentStakedAmount.Denom) | ||
| riskFactor := s.App.SuperfluidKeeper.CalculateRiskFactor(s.Ctx, btcDenom) | ||
| twapStartTime := s.Ctx.BlockTime().Add(-5 * time.Minute) | ||
| price, err := s.App.TwapKeeper.GetMultiPoolArithmeticTwapToNow(s.Ctx, []*poolmanagertypes.SwapAmountInRoute{{PoolId: nextPoolId, TokenOutDenom: bondDenom}}, btcDenom, bondDenom, twapStartTime) | ||
| equivalentAmount := riskFactor.Mul(osmomath.NewDec(btcStakeAmount.Int64())).Mul(price) | ||
| s.Require().NoError(err) | ||
| s.Require().Equal(equivalentAmount, osmomath.NewDec(res.SuperfluidDelegationRecords[0].EquivalentStakedAmount.Amount.Int64())) | ||
| s.Require().Equal(appparams.BaseCoinUnit, res.TotalEquivalentStakedAmount.Denom) | ||
| s.Require().Equal(equivalentAmount, osmomath.NewDec(res.TotalEquivalentStakedAmount.Amount.Int64())) | ||
| s.Require().Equal(appparams.BaseCoinUnit, res.TotalEquivalentNonOsmoStakedAmount.Denom) | ||
| s.Require().Equal(equivalentAmount, osmomath.NewDec(res.TotalEquivalentNonOsmoStakedAmount.Amount.Int64())) |
There was a problem hiding this comment.
The calculation of equivalent staked amounts using TWAP prices and risk factors is a significant addition. It's important to ensure that these calculations are accurate and reflect the true market conditions. Consider adding more detailed logging or debugging information to help trace these calculations in a production environment.
+ fmt.Println("Detailed TWAP calculation:", price)Committable suggestion was skipped due to low confidence.
x/superfluid/keeper/stake.go
Outdated
| func (k Keeper) ValidateNativeAsset(asset types.SuperfluidAsset) error { | ||
| if asset.AssetType == types.SuperfluidAssetTypeNative { | ||
| if len(asset.PriceRoute) == 0 || | ||
| strings.TrimSpace(asset.PriceRoute[0].TokenOutDenom) == "" { | ||
| return errorsmod.Wrap(types.ErrNonSuperfluidAsset, "asset is not properly configured for superfluid staking: missing or empty price route") | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Ensure proper configuration for native superfluid assets.
The function ValidateNativeAsset checks if the asset type is native and then validates the PriceRoute. However, the error message could be more specific about what is exactly missing or misconfigured. Consider enhancing the error message to improve clarity and debugging efficiency.
- return errorsmod.Wrap(types.ErrNonSuperfluidAsset, "asset is not properly configured for superfluid staking: missing or empty price route")
+ return errorsmod.Wrap(types.ErrNonSuperfluidAsset, "asset is not properly configured for superfluid staking: missing or empty price route in the first element")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.
| func (k Keeper) ValidateNativeAsset(asset types.SuperfluidAsset) error { | |
| if asset.AssetType == types.SuperfluidAssetTypeNative { | |
| if len(asset.PriceRoute) == 0 || | |
| strings.TrimSpace(asset.PriceRoute[0].TokenOutDenom) == "" { | |
| return errorsmod.Wrap(types.ErrNonSuperfluidAsset, "asset is not properly configured for superfluid staking: missing or empty price route") | |
| } | |
| } | |
| return nil | |
| } | |
| func (k Keeper) ValidateNativeAsset(asset types.SuperfluidAsset) error { | |
| if asset.AssetType == types.SuperfluidAssetTypeNative { | |
| if len(asset.PriceRoute) == 0 || | |
| strings.TrimSpace(asset.PriceRoute[0].TokenOutDenom) == "" { | |
| return errorsmod.Wrap(types.ErrNonSuperfluidAsset, "asset is not properly configured for superfluid staking: missing or empty price route in the first element") | |
| } | |
| } | |
| return nil | |
| } |
x/superfluid/keeper/stake_test.go
Outdated
| params, _ := s.App.StakingKeeper.GetParams(s.Ctx) | ||
| bondDenom := params.BondDenom |
There was a problem hiding this comment.
The retrieval of bondDenom from staking parameters is repeated in multiple test cases. Consider refactoring this into a helper function to reduce code duplication.
- params, _ := s.App.StakingKeeper.GetParams(s.Ctx)
- bondDenom := params.BondDenom
+ bondDenom := s.getBondDenom()And add a new method:
func (s *KeeperTestSuite) getBondDenom() string {
params, _ := s.App.StakingKeeper.GetParams(s.Ctx)
return params.BondDenom
}
x/superfluid/keeper/stake_test.go
Outdated
| @@ -246,7 +250,7 @@ func (s *KeeperTestSuite) TestValidateLockForSFDelegate() { | |||
| Duration: time.Hour * 24 * 21, | |||
| ID: 1, | |||
| }, | |||
| superfluidAssetToSet: types.SuperfluidAsset{Denom: "foo", AssetType: types.SuperfluidAssetTypeNative, PricePoolId: 1}, | |||
| superfluidAssetToSet: types.SuperfluidAsset{Denom: "foo", AssetType: types.SuperfluidAssetTypeNative, PriceRoute: []*poolmanagertypes.SwapAmountInRoute{{PoolId: 1, TokenOutDenom: bondDenom}}}, | |||
There was a problem hiding this comment.
The test cases for "valid native lock" and "invalid native lock - asset not set" use the same superfluidAssetToSet setup. This redundancy could be streamlined by setting up the superfluidAssetToSet before the test cases if it does not change.
- superfluidAssetToSet: types.SuperfluidAsset{Denom: "foo", AssetType: types.SuperfluidAssetTypeNative, PriceRoute: []*poolmanagertypes.SwapAmountInRoute{{PoolId: 1, TokenOutDenom: bondDenom}}},
+ superfluidAssetToSet: commonAssetSetupAnd define commonAssetSetup before the tests:
commonAssetSetup := types.SuperfluidAsset{Denom: "foo", AssetType: types.SuperfluidAssetTypeNative, PriceRoute: []*poolmanagertypes.SwapAmountInRoute{{PoolId: 1, TokenOutDenom: bondDenom}}}Also applies to: 264-264
x/superfluid/keeper/gov/gov_test.go
Outdated
| Denom: "btc", | ||
| AssetType: types.SuperfluidAssetTypeNative, | ||
| PriceRoute: []*poolmanagertypes.SwapAmountInRoute{{PoolId: 1, TokenOutDenom: "stake"}}, |
There was a problem hiding this comment.
Validate the PriceRoute field in SuperfluidAsset for native assets.
The PriceRoute field should not typically be set for native assets as their price is not derived from a pool. This could potentially introduce bugs or unexpected behavior. Consider adding validation logic to prevent or handle this scenario appropriately.
x/superfluid/keeper/epoch_test.go
Outdated
| params, _ := s.App.StakingKeeper.GetParams(s.Ctx) | ||
| params.BondDenom = appparams.BaseCoinUnit | ||
| err := s.App.StakingKeeper.SetParams(s.Ctx, params) | ||
| s.Require().NoError(err) |
There was a problem hiding this comment.
The parameters for the staking keeper are being reset within each test case. Consider refactoring to avoid redundancy and potential side effects across tests.
- params, _ := s.App.StakingKeeper.GetParams(s.Ctx)
- params.BondDenom = appparams.BaseCoinUnit
- err := s.App.StakingKeeper.SetParams(s.Ctx, params)
- s.Require().NoError(err)
+ // Refactor to set parameters once in a setup method if applicableCommittable suggestion was skipped due to low confidence.
x/superfluid/keeper/gov/gov.go
Outdated
| if err := k.ValidateNativeAsset(asset); err != nil { | ||
| return fmt.Errorf("native asset (%s) must have a price route", asset.Denom) |
There was a problem hiding this comment.
Consider enhancing the error message to include more context about the failure.
- return fmt.Errorf("native asset (%s) must have a price route", asset.Denom)
+ return fmt.Errorf("validation failed for native asset (%s): a price route is required", asset.Denom)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.
| if err := k.ValidateNativeAsset(asset); err != nil { | |
| return fmt.Errorf("native asset (%s) must have a price route", asset.Denom) | |
| if err := k.ValidateNativeAsset(asset); err != nil { | |
| return fmt.Errorf("validation failed for native asset (%s): a price route is required", asset.Denom) |
x/superfluid/keeper/gov/gov.go
Outdated
| @@ -40,7 +40,7 @@ func HandleRemoveSuperfluidAssetsProposal(ctx sdk.Context, k keeper.Keeper, p *t | |||
| return err | |||
| } | |||
| dummyAsset := types.SuperfluidAsset{} | |||
| if asset == dummyAsset { | |||
| if asset.Equal(dummyAsset) { | |||
There was a problem hiding this comment.
Consider using a more direct method to check for asset existence instead of comparing to a dummy object.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (8)
CHANGELOG.md (8)
Line range hint
1070-1070: Consider adjusting the heading level from h4 to h3 for consistency.
Line range hint
1082-1082: Consider adjusting the heading level from h4 to h3 for consistency.
Line range hint
1091-1091: Consider adjusting the heading level from h4 to h3 for consistency.
Line range hint
1096-1096: Consider adjusting the heading level from h4 to h3 for consistency.
Line range hint
1148-1148: Consider adjusting the heading level from h4 to h3 for consistency.
Line range hint
409-409: Remove the trailing punctuation from the heading.
Line range hint
672-672: Avoid using bare URLs. Consider using markdown link syntax or providing additional context.Also applies to: 730-730
Line range hint
1311-1311: Ensure that there are no empty links in the document.
What is the purpose of the change
Queries now take non-pool assets into account
Testing and Verifying
Updated necessary tests
Documentation and Release Note
Unreleasedsection ofCHANGELOG.md?Where is the change documented?
x/{module}/README.md)