[NonPoolSuperfluid] Add risk factors to individual denoms. #8250
Conversation
…ative is properly tested
…ns aren't returned after delegation ends
…ependent-risk-factor
github-actions
bot
added
C:CLI
C:x/incentives
C:x/superfluid
C:app-wiring
|
should I rename RiskFactor to DiscountFactor? |
proto/osmosis/superfluid/tx.proto
Outdated
| string sender = 1 [ (gogoproto.moretags) = "yaml:\"sender\"" ]; | ||
| string denom = 2 [ (gogoproto.moretags) = "yaml:\"denom\"" ]; | ||
| string risk_factor = 3 [ (gogoproto.moretags) = "yaml:\"risk_factor\"" ]; |
There was a problem hiding this comment.
I think this needs a sdk.Dec tag right?
| message DenomRiskFactor { | ||
| // superfluid asset denom, can be LP token or native token | ||
| string denom = 1; | ||
| string risk_factor = 2 [ |
There was a problem hiding this comment.
Can a brief explanation of what the risk factor is be added here?
x/superfluid/keeper/epoch.go
Outdated
| @@ -151,7 +151,7 @@ func (k Keeper) UpdateOsmoEquivalentMultipliers(ctx sdk.Context, asset types.Sup | |||
| return errors.New("osmo should not be a superfluid asset. It can be staked natively") | |||
| } | |||
| // get the twap price of the native asset in osmo | |||
| startTime := k.ek.GetEpochInfo(ctx, k.GetEpochIdentifier(ctx)).StartTime | |||
| startTime := k.ek.GetEpochInfo(ctx, k.GetEpochIdentifier(ctx)).StartTime // TODO: do 5 mins instead of 1 epoch | |||
There was a problem hiding this comment.
What does this TODO mean? You want the twap over 5 minutes rather than the full epoch? Is there any reason we cant just do that here in this PR?
There was a problem hiding this comment.
yeah, that's what it means. We can do it here, but I wanted to make it a param so I thought it'd be nice to have its own PR
| {"happy path", govAddr, "something", "0.1", ""}, | ||
| {"happy path with zero", govAddr, "something", "0", ""}, | ||
| {"bad gov addr", "osmo1herasn5ewvv9acpujdmqxz698y849aq9ucsccl", "something", "0", "only the governance module is allowed to execute this message"}, | ||
| {"bad gov addr 2", "something else", "something", "0", "invalid sender address (decoding bech32 failed"}, | ||
| {"bad denom", govAddr, "", "0", "denom cannot be empty"}, | ||
| {"bad risk factor", govAddr, "something", "NaN", "invalid risk factor (failed to set decimal string"}, |
There was a problem hiding this comment.
nit: Since this is a new test, can this be refactored to show the struct name next to each entry? It makes it 100x easier to parse rather than matching each field up to its respective struct field.
Also, can the expectedError be of type error, so we can use error types instead of hard coding the text of the error?
There was a problem hiding this comment.
I like using strings for the error because that way I can do partial matching (Contains). Other than that, yeah, we can refactor
| tests := []riskFactorTest{ | ||
| {"test risk factor not set ", "random", "", "btc", 100, 50}, | ||
| {"test risk factor set high", "btc", "0.8", "btc", 100, 20}, | ||
| {"test risk factor set low", "btc", "0.1", "btc", 100, 90}, | ||
| {"test risk set diff query", "gamm/pool/1", "0.1", "btc", 100, 50}, // default | ||
| {"test risk set same query", "gamm/pool/1", "0.1", "gamm/pool/1", 100, 90}, // set |
There was a problem hiding this comment.
Same requests with these, would be nice to have struct names in front of each input to make understanding each test case easier.
There was a problem hiding this comment.
doesn't your editor display the types inline? Mine looks like this:
WalkthroughThe recent updates to the Osmosis superfluid module bring enhancements for managing risk factors tied to specific denominations. Notable changes include new message types and RPC methods for setting and unsetting denomination risk factors, updated risk-adjusted value computations, and related test scenarios. Additionally, the time duration for adjusting Osmo equivalent multipliers has shifted from 1 epoch to 5 minutes. Changes
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (2)
x/superfluid/keeper/superfluid_asset_test.go (1)
55-60: Consider adding struct field names in the test cases for clarity.This will improve readability and maintainability, especially for new contributors or when revisiting this code after some time.
proto/osmosis/superfluid/superfluid.proto (1)
108-108: Add a description for therisk_factorfield.It would be beneficial to include a brief comment explaining what the
risk_factorrepresents and how it is used within the system. This will enhance the understandability of the code for developers and maintainers.
| func (s *KeeperTestSuite) TestSetDenomRiskFactors() { | ||
| s.SetupTest() | ||
|
|
||
| msgServer := keeper.NewMsgServerImpl(s.App.SuperfluidKeeper) | ||
| c := sdk.WrapSDKContext(s.Ctx) | ||
|
|
||
| govAddr := s.App.AccountKeeper.GetModuleAccount(s.Ctx, govtypes.ModuleName).GetAddress().String() | ||
|
|
||
| // Let's turn this into a table test | ||
| setDenomRiskFactorTests := []struct { | ||
| name string | ||
| sender string | ||
| denom string | ||
| riskFactor string | ||
| expectedError string | ||
| }{ | ||
| {"happy path", govAddr, "something", "0.1", ""}, | ||
| {"happy path with zero", govAddr, "something", "0", ""}, | ||
| {"bad gov addr", "osmo1herasn5ewvv9acpujdmqxz698y849aq9ucsccl", "something", "0", "only the governance module is allowed to execute this message"}, | ||
| {"bad gov addr 2", "something else", "something", "0", "invalid sender address (decoding bech32 failed"}, | ||
| {"bad denom", govAddr, "", "0", "denom cannot be empty"}, | ||
| {"bad risk factor", govAddr, "something", "NaN", "invalid risk factor (failed to set decimal string"}, | ||
| } | ||
|
|
||
| for _, test := range setDenomRiskFactorTests { | ||
| s.Run(test.name, func() { | ||
| s.SetupTest() | ||
|
|
||
| msg := &types.MsgSetDenomRiskFactor{ | ||
| Sender: test.sender, | ||
| Denom: test.denom, | ||
| RiskFactor: test.riskFactor, | ||
| } | ||
| err := msg.ValidateBasic() | ||
| if err == nil { | ||
| _, err = msgServer.SetDenomRiskFactor(c, msg) | ||
| } | ||
|
|
||
| if test.expectedError != "" { | ||
| s.Require().Error(err) | ||
| s.Require().ErrorContains(err, test.expectedError) | ||
| } else { | ||
| s.Require().NoError(err) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider refactoring the test cases into a separate function for clarity and reusability.
This will improve the readability and maintainability of the test code. Here's a suggested refactor:
func getSetDenomRiskFactorTests(govAddr string) []struct {
name string
sender string
denom string
riskFactor string
expectedError string
} {
return []struct {
name string
sender string
denom string
riskFactor string
expectedError string
}{
{"happy path", govAddr, "something", "0.1", ""},
{"happy path with zero", govAddr, "something", "0", ""},
{"bad gov addr", "osmo1herasn5ewvv9acpujdmqxz698y849aq9ucsccl", "something", "0", "only the governance module is allowed to execute this message"},
{"bad gov addr 2", "something else", "something", "0", "invalid sender address (decoding bech32 failed"},
{"bad denom", govAddr, "", "0", "denom cannot be empty"},
{"bad risk factor", govAddr, "something", "NaN", "invalid risk factor (failed to set decimal string"},
}
}
x/superfluid/keeper/epoch.go
Outdated
| @@ -151,7 +151,7 @@ func (k Keeper) UpdateOsmoEquivalentMultipliers(ctx sdk.Context, asset types.Sup | |||
| return errors.New("osmo should not be a superfluid asset. It can be staked natively") | |||
| } | |||
| // get the twap price of the native asset in osmo | |||
| startTime := k.ek.GetEpochInfo(ctx, k.GetEpochIdentifier(ctx)).StartTime | |||
| startTime := k.ek.GetEpochInfo(ctx, k.GetEpochIdentifier(ctx)).StartTime // TODO: do 5 mins instead of 1 epoch | |||
There was a problem hiding this comment.
Consider resolving the TODO about making the start time configurable or create a tracking issue.
Would you like me to help by creating a GitHub issue to track this enhancement?
| func (k Keeper) SetDenomRiskFactor(ctx sdk.Context, denom string, riskFactor osmomath.Dec) error { | ||
| store := ctx.KVStore(k.storeKey) | ||
| prefixStore := prefix.NewStore(store, types.KeyPrefixDenomRiskFactor) | ||
| riskFactorRecord := types.DenomRiskFactor{ | ||
| RiskFactor: riskFactor, | ||
| } | ||
| bz, err := proto.Marshal(&riskFactorRecord) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| prefixStore.Set([]byte(denom), bz) | ||
| return nil | ||
| } | ||
|
|
||
| func (k Keeper) DeleteDenomRiskFactor(ctx sdk.Context, denom string) { | ||
| store := ctx.KVStore(k.storeKey) | ||
| prefixStore := prefix.NewStore(store, types.KeyPrefixDenomRiskFactor) | ||
| prefixStore.Delete([]byte(denom)) | ||
| } | ||
|
|
||
| func (k Keeper) GetDenomRiskFactor(ctx sdk.Context, denom string) (osmomath.Dec, bool) { | ||
| store := ctx.KVStore(k.storeKey) | ||
| prefixStore := prefix.NewStore(store, types.KeyPrefixDenomRiskFactor) | ||
| bz := prefixStore.Get([]byte(denom)) | ||
| if bz == nil { | ||
| return osmomath.ZeroDec(), false | ||
| } | ||
| denomRiskFactor := types.DenomRiskFactor{} | ||
| err := proto.Unmarshal(bz, &denomRiskFactor) | ||
| if err != nil { | ||
| return osmomath.ZeroDec(), false | ||
| } | ||
| return denomRiskFactor.RiskFactor, true | ||
| } | ||
|
|
||
| func (k Keeper) GetAllDenomRiskFactors(ctx sdk.Context) []types.DenomRiskFactor { | ||
| store := ctx.KVStore(k.storeKey) | ||
| prefixStore := prefix.NewStore(store, types.KeyPrefixDenomRiskFactor) | ||
| iterator := prefixStore.Iterator(nil, nil) | ||
| defer iterator.Close() | ||
|
|
||
| denomRiskFactors := []types.DenomRiskFactor{} | ||
| for ; iterator.Valid(); iterator.Next() { | ||
| denomRiskFactor := types.DenomRiskFactor{} | ||
|
|
||
| err := proto.Unmarshal(iterator.Value(), &denomRiskFactor) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| denomRiskFactors = append(denomRiskFactors, denomRiskFactor) | ||
| } | ||
| return denomRiskFactors | ||
| } |
There was a problem hiding this comment.
Consider refactoring these functions into a store.go file.
Moving SetDenomRiskFactor, DeleteDenomRiskFactor, and GetDenomRiskFactor to a store.go file could improve the organization of the codebase. This aligns with common practices where store-related functions are kept separate from business logic, enhancing maintainability.
What is the purpose of the change
Add independent risk factors to different superfluid denoms
Testing and Verifying
Added necessary tests
Documentation and Release Note
Unreleasedsection ofCHANGELOG.md?Where is the change documented?
x/{module}/README.md)