fix: last updated setting in x/marketmap keeper [DO NOT MERGE]
#799
Conversation
aljo242
requested review from
Eric-Warehime,
technicallyty,
wesl-ee and
zrbecker
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #799 +/- ##
==========================================
+ Coverage 57.39% 57.40% +0.01%
==========================================
Files 214 214
Lines 14844 14850 +6
==========================================
+ Hits 8520 8525 +5
Misses 5710 5710
- Partials 614 615 +1 ☔ View full report in Codecov by Sentry. |
tests/integration/connect_setup.go
Outdated
| // create the mm client | ||
| client := mmtypes.NewQueryClient(cc) | ||
|
|
||
| ctx := context.Background() |
There was a problem hiding this comment.
I know this is only used in tests, but can we just pass a ctx to this function and use it. Ideally a context is defined at the beginning of tests that we can use, and it looks like there is a context for us to use in this case, so we should avoid creating this context if possible.
tests/integration/connect_setup.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if resp.TxResult.Code != abcitypes.CodeTypeOK { | ||
| return fmt.Errorf(resp.TxResult.Log) | ||
| if txResp.TxResult.Code != abcitypes.CodeTypeOK { |
There was a problem hiding this comment.
Do proposals pass immediately with interchaintest?
There was a problem hiding this comment.
No, but the result of a proposal or submitted Tx will be TypeOK if it landed successfully
| @@ -89,7 +89,18 @@ func (k *Keeper) GetMarket(ctx context.Context, tickerStr string) (types.Market, | |||
|
|
|||
| // setMarket sets a market. | |||
| func (k *Keeper) setMarket(ctx context.Context, market types.Market) error { | |||
| return k.markets.Set(ctx, types.TickerString(market.Ticker.String()), market) | |||
| if err := k.markets.Set(ctx, types.TickerString(market.Ticker.String()), market); err != nil { | |||
There was a problem hiding this comment.
Just curious, what are the properties of this cosmos-sdk collections map? Does it handle stuff to ensure things are deterministic? Any idea of the specifics?
| @@ -85,7 +85,7 @@ func (ms msgServer) UpsertMarkets(goCtx context.Context, msg *types.MsgUpsertMar | |||
| return nil, err | |||
| } | |||
|
|
|||
| return &types.MsgUpsertMarketsResponse{}, ms.k.SetLastUpdated(ctx, uint64(ctx.BlockHeight())) //nolint:gosec | |||
There was a problem hiding this comment.
For these functions, SetLastUpdated will not get called many times per upsert potentially. Are there gas consequences of this? Is that a problem?
There was a problem hiding this comment.
There are gas consequences because it is called many times yes.
This isn't a problem per-se as these transactions are only executed by gov / the mmu auth, but it is another reason why this PR is state breaking
aljo242
changed the title
x/marketmap keeperx/marketmap keeper [DO NOT MERGE]
zrbecker
requested review from
zrbecker
and removed request for
zrbecker,
Eric-Warehime,
technicallyty and
wesl-ee
Closes CON-1839
SetLastUpdatedfor anySetorRemovecalls in the keeper. (previously these calls were only done during the MsgServer which meant that hooks, or any keeper calls that are not part of a MsgServer call were not updating LastUpdated)LastUpdatedfield. Use randomized blockheights instead of constantsNote, this fix is only for on-chain code, so a full upgrade would be needed. This is also state-breaking, so we will need to make a new
v3.xrelease as anyv1orv2version of the code will result in disparate states (last updated will be updated differently).I will make another PR that will only updates the sidecar, which can be rolled out more quickly.