Twap: Add twapkeeper wiring #2193
Conversation
github-actions
bot
added
C:app-wiring
| @@ -121,6 +123,7 @@ func appModules( | |||
| params.NewAppModule(*app.ParamsKeeper), | |||
| app.TransferModule, | |||
| gamm.NewAppModule(appCodec, *app.GAMMKeeper, app.AccountKeeper, app.BankKeeper), | |||
| twap.NewAppModule(*app.TwapKeeper), | |||
There was a problem hiding this comment.
Something cool is that with the structure of keeper and module.go being defined in the same file, no other keepers need to be passed in.
Also AppCodec isn't needed in modules that don't serialize interfaces
| denoms, err := s.App.GAMMKeeper.GetPoolDenoms(s.Ctx, poolId) | ||
| s.Require().NoError(err) | ||
|
|
||
| swapIn := sdk.NewCoins(sdk.NewCoin(denoms[0], sdk.NewInt(1000))) |
There was a problem hiding this comment.
Do you think it would be useful to have these as parameters so that the caller can customize them?
I find that frequently in GAMM tests, we have to hard code some parameters because such test helpers are not customizable.
There was a problem hiding this comment.
Maybe we could let it take MsgSwapExactAmountIn for full control over how the caller wants the swap to behave?
There was a problem hiding this comment.
The point of this one was to not have to think about anything, I think a second helper that does that would be helpful
| TokenIn: swapIn[0], | ||
| TokenOutMinAmount: sdk.ZeroInt(), | ||
| } | ||
| // TODO: switch to message |
There was a problem hiding this comment.
let's create an issue for this please
| @@ -241,6 +244,12 @@ func (appKeepers *AppKeepers) InitNormalKeepers( | |||
| appKeepers.AccountKeeper, appKeepers.BankKeeper, appKeepers.DistrKeeper) | |||
| appKeepers.GAMMKeeper = &gammKeeper | |||
|
|
|||
| appKeepers.TwapKeeper = twap.NewKeeper( | |||
There was a problem hiding this comment.
Asking to learn - since TWAP has its own keeper why would we not have a separate module for it? Isn't this inconsistent with the cosmos-sdk conventions?
There was a problem hiding this comment.
Yeah we can move it to its own module, the way its written this is really just a folder move
| @@ -197,6 +198,12 @@ func (k Keeper) DeletePool(ctx sdk.Context, poolId uint64) error { | |||
| // return nil | |||
| // } | |||
|
|
|||
| func (k Keeper) GetPoolDenoms(ctx sdk.Context, poolId uint64) ([]string, error) { | |||
| @@ -197,6 +198,12 @@ func (k Keeper) DeletePool(ctx sdk.Context, poolId uint64) error { | |||
| // return nil | |||
| // } | |||
|
|
|||
| func (k Keeper) GetPoolDenoms(ctx sdk.Context, poolId uint64) ([]string, error) { | |||
| pool, err := k.GetPoolAndPoke(ctx, poolId) | |||
| @@ -197,6 +198,12 @@ func (k Keeper) DeletePool(ctx sdk.Context, poolId uint64) error { | |||
| // return nil | |||
| // } | |||
|
|
|||
| func (k Keeper) GetPoolDenoms(ctx sdk.Context, poolId uint64) ([]string, error) { | |||
There was a problem hiding this comment.
Also, let's make a test for this please with a success and an error case
| return &Keeper{storeKey: storeKey, transientKey: transientKey, paramSpace: paramSpace, ammkeeper: ammKeeper} | ||
| } | ||
|
|
||
| // TODO: make this read from a parameter, or hardcode it. |
|
I can help with creating issues / PRs for TODOs and comments that are not going to be immediately addressed. @ValarDragon do you mind resolving comments that you're planning on addressing in follow-ups, please? I can take care of the rest |
|
|
||
| // TODO: make this read from a parameter, or hardcode it. | ||
| func (k *Keeper) PruneEpochIdentifier(ctx sdk.Context) string { | ||
| return "daily" |
There was a problem hiding this comment.
Should it be "day" and not "daily" according to the epochs parameter?
There was a problem hiding this comment.
oh woops, yeah it should
What is the purpose of the change
Extract from #2168 , does the app wiring for twap into the Osmosis app
Brief Changelog
Testing and Verifying
Wiring across an upgrade boundary is covered by existing test e2e
Documentation and Release Note
Unreleasedsection inCHANGELOG.md? needs changelog entry