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

Jump to bottom

fix: last updated setting in x/marketmap keeper [DO NOT MERGE] #799

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from
Draft

fix: last updated setting in x/marketmap keeper [DO NOT MERGE] #799

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
596b64d
fix
aljo242 Oct 16, 2024
d240fb3
update on delete
aljo242 Oct 16, 2024
32b8c05
test
aljo242 Oct 16, 2024
c99bcb4
always use random blockheights
aljo242 Oct 16, 2024
b74b975
update
aljo242 Oct 16, 2024
7da6a3c
cute
aljo242 Oct 16, 2024
534986e
add more checks
aljo242 Oct 16, 2024
db8b606
lint
aljo242 Oct 16, 2024
98b92b2
Merge branch 'main' into fix/last-updated
aljo242 Oct 17, 2024
2d55818
Merge branch 'main' into fix/last-updated
aljo242 Oct 17, 2024
bad3330
Merge branch 'main' into fix/last-updated
aljo242 Oct 22, 2024
0da86d9
clean up
aljo242 Oct 22, 2024
8054d9d
fix
aljo242 Oct 22, 2024
2d6dc3a
Merge branch 'main' into fix/last-updated
aljo242 Oct 23, 2024
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
15 changes: 13 additions & 2 deletions x/marketmap/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,18 @@

// 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 {
Copy link
Member

Choose a reason for hiding this comment

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

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?

return err
}

Check warning on line 94 in x/marketmap/keeper/keeper.go

View check run for this annotation

Codecov / codecov/patch

x/marketmap/keeper/keeper.go#L93-L94 

Added lines #L93 - L94 were not covered by tests

// always set LastUpdated when the market is updated.
return k.setLastUpdatedFromContext(ctx)
}

// setLastUpdatedFromContext calls SetLastUpdated using the ctx BlockHeight() value.
func (k *Keeper) setLastUpdatedFromContext(ctx context.Context) error {
sdkCtx := sdk.UnwrapSDKContext(ctx)
return k.SetLastUpdated(ctx, uint64(sdkCtx.BlockHeight())) //nolint:gosec
}

// EnableMarket sets the Enabled field of a Market Ticker to true.
Expand Down Expand Up @@ -186,7 +197,7 @@
return false, err
}

return true, nil
return true, k.setLastUpdatedFromContext(ctx)
}

// HasMarket checks if a market exists in the store.
Expand Down
108 changes: 78 additions & 30 deletions x/marketmap/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,19 @@ package keeper_test
import (
"testing"

"github.com/skip-mev/chaintestutil/sample"

oraclekeeper "github.com/skip-mev/connect/v2/x/oracle/keeper"
oracletypes "github.com/skip-mev/connect/v2/x/oracle/types"

storetypes "cosmossdk.io/store/types"
"github.com/cosmos/cosmos-sdk/runtime"
"github.com/cosmos/cosmos-sdk/testutil"
sdk "github.com/cosmos/cosmos-sdk/types"
moduletestutil "github.com/cosmos/cosmos-sdk/types/module/testutil"
"github.com/skip-mev/chaintestutil/sample"
"github.com/stretchr/testify/suite"

connecttypes "github.com/skip-mev/connect/v2/pkg/types"
"github.com/skip-mev/connect/v2/x/marketmap/keeper"
"github.com/skip-mev/connect/v2/x/marketmap/types"
oraclekeeper "github.com/skip-mev/connect/v2/x/oracle/keeper"
oracletypes "github.com/skip-mev/connect/v2/x/oracle/types"
)

var r = sample.Rand()
Expand Down Expand Up @@ -177,28 +175,36 @@ var (
)

func (s *KeeperTestSuite) TestGets() {
testBlockHeight := r.Int63()
ctx := s.ctx.WithBlockHeight(testBlockHeight)
defer func() {
lastUpdated, err := s.keeper.GetLastUpdated(ctx)
s.Require().NoError(err)
s.Require().Equal(lastUpdated, uint64(testBlockHeight)) //nolint:gosec
}()

s.Run("get empty market map", func() {
got, err := s.keeper.GetAllMarkets(s.ctx)
got, err := s.keeper.GetAllMarkets(ctx)
s.Require().NoError(err)
s.Require().Equal(map[string]types.Market{}, got)
})

s.Run("setup initial markets", func() {
for _, market := range markets {
s.Require().NoError(s.keeper.CreateMarket(s.ctx, market))
s.Require().NoError(s.keeper.CreateMarket(ctx, market))
}

s.Run("unable to set markets again", func() {
for _, market := range markets {
s.Require().ErrorIs(s.keeper.CreateMarket(s.ctx, market), types.NewMarketAlreadyExistsError(types.TickerString(market.Ticker.String())))
s.Require().ErrorIs(s.keeper.CreateMarket(ctx, market), types.NewMarketAlreadyExistsError(types.TickerString(market.Ticker.String())))
}
})

s.Require().NoError(s.keeper.ValidateState(s.ctx, markets))
s.Require().NoError(s.keeper.ValidateState(ctx, markets))
})

s.Run("get all tickers", func() {
got, err := s.keeper.GetAllMarkets(s.ctx)
got, err := s.keeper.GetAllMarkets(ctx)
s.Require().NoError(err)

s.Require().Equal(len(markets), len(got))
Expand Down Expand Up @@ -322,51 +328,93 @@ func (s *KeeperTestSuite) TestInvalidCreateDisabledNormalizeBy() {
}

func (s *KeeperTestSuite) TestDeleteMarket() {
testBlockHeight := r.Int63()
ctx := s.ctx.WithBlockHeight(testBlockHeight)
defer func() {
lastUpdated, err := s.keeper.GetLastUpdated(ctx)
s.Require().NoError(err)
s.Require().Equal(lastUpdated, uint64(testBlockHeight)) //nolint:gosec
}()

// create a valid markets
btcCopy := btcusdt
btcCopy.Ticker.Enabled = true
s.Require().NoError(s.keeper.CreateMarket(s.ctx, btcCopy))
s.Require().NoError(s.keeper.CreateMarket(ctx, btcCopy))

// invalid delete will return nil - idempotent
deleted, err := s.keeper.DeleteMarket(s.ctx, "foobar")
deleted, err := s.keeper.DeleteMarket(ctx, "foobar")
s.Require().NoError(err)
s.Require().False(deleted)

// cannot delete enabled markets
deleted, err = s.keeper.DeleteMarket(s.ctx, btcCopy.Ticker.String())
deleted, err = s.keeper.DeleteMarket(ctx, btcCopy.Ticker.String())
s.Require().Error(err)
s.Require().False(deleted)

// disable market
btcCopy.Ticker.Enabled = false
s.Require().NoError(s.keeper.UpdateMarket(s.ctx, btcCopy))
s.Require().NoError(s.keeper.UpdateMarket(ctx, btcCopy))

// delete disabled markets
deleted, err = s.keeper.DeleteMarket(s.ctx, btcCopy.Ticker.String())
deleted, err = s.keeper.DeleteMarket(ctx, btcCopy.Ticker.String())
s.Require().NoError(err)
s.Require().True(deleted)

_, err = s.keeper.GetMarket(s.ctx, btcCopy.Ticker.String())
_, err = s.keeper.GetMarket(ctx, btcCopy.Ticker.String())
s.Require().Error(err)
}

func (s *KeeperTestSuite) TestEnableDisableMarket() {
// create a valid markets
s.Require().NoError(s.keeper.CreateMarket(s.ctx, btcusdt))
s.Run("create a valid markets", func() {
testBlockHeight := r.Int63()
ctx := s.ctx.WithBlockHeight(testBlockHeight)
defer func() {
lastUpdated, err := s.keeper.GetLastUpdated(ctx)
s.Require().NoError(err)
s.Require().Equal(lastUpdated, uint64(testBlockHeight)) //nolint:gosec
}()

s.Require().NoError(s.keeper.CreateMarket(ctx, btcusdt))
})

// invalid enable/disable fails
s.Require().Error(s.keeper.EnableMarket(s.ctx, "foobar"))
s.Require().Error(s.keeper.DisableMarket(s.ctx, "foobar"))
s.Run("invalid enable/disable fails", func() {
testBlockHeight := r.Int63()
ctx := s.ctx.WithBlockHeight(testBlockHeight)

// valid enable works
s.Require().NoError(s.keeper.EnableMarket(s.ctx, btcusdt.Ticker.String()))
market, err := s.keeper.GetMarket(s.ctx, btcusdt.Ticker.String())
s.Require().NoError(err)
s.Require().True(market.Ticker.Enabled)
s.Require().Error(s.keeper.EnableMarket(ctx, "foobar"))
s.Require().Error(s.keeper.DisableMarket(ctx, "foobar"))
})

// valid disable works
s.Require().NoError(s.keeper.DisableMarket(s.ctx, btcusdt.Ticker.String()))
market, err = s.keeper.GetMarket(s.ctx, btcusdt.Ticker.String())
s.Require().NoError(err)
s.Require().False(market.Ticker.Enabled)
s.Run("valid enable works", func() {
testBlockHeight := r.Int63()
ctx := s.ctx.WithBlockHeight(testBlockHeight)
defer func() {
lastUpdated, err := s.keeper.GetLastUpdated(ctx)
s.Require().NoError(err)
s.Require().Equal(lastUpdated, uint64(testBlockHeight)) //nolint:gosec
}()

// valid enable works
s.Require().NoError(s.keeper.EnableMarket(ctx, btcusdt.Ticker.String()))
market, err := s.keeper.GetMarket(ctx, btcusdt.Ticker.String())
s.Require().NoError(err)
s.Require().True(market.Ticker.Enabled)
})

s.Run("valid enable works", func() {
testBlockHeight := r.Int63()
ctx := s.ctx.WithBlockHeight(testBlockHeight)
defer func() {
lastUpdated, err := s.keeper.GetLastUpdated(ctx)
s.Require().NoError(err)
s.Require().Equal(lastUpdated, uint64(testBlockHeight)) //nolint:gosec
}()

// valid disable works
s.Require().NoError(s.keeper.DisableMarket(ctx, btcusdt.Ticker.String()))
market, err := s.keeper.GetMarket(ctx, btcusdt.Ticker.String())
s.Require().NoError(err)
s.Require().False(market.Ticker.Enabled)
})
}
6 changes: 3 additions & 3 deletions x/marketmap/keeper/msg_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

For these functions, SetLastUpdated will not get called many times per upsert potentially. Are there gas consequences of this? Is that a problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

return &types.MsgUpsertMarketsResponse{}, nil
}

// CreateMarkets updates the marketmap by creating markets from the given message. All updates are made to the market
Expand Down Expand Up @@ -126,7 +126,7 @@ func (ms msgServer) CreateMarkets(goCtx context.Context, msg *types.MsgCreateMar
return nil, fmt.Errorf("invalid state resulting from update: %w", err)
}

return &types.MsgCreateMarketsResponse{}, ms.k.SetLastUpdated(ctx, uint64(ctx.BlockHeight())) //nolint:gosec
return &types.MsgCreateMarketsResponse{}, nil
}

// UpdateMarkets updates the marketmap by updating markets from the given message. All updates are made to the market
Expand Down Expand Up @@ -166,7 +166,7 @@ func (ms msgServer) UpdateMarkets(goCtx context.Context, msg *types.MsgUpdateMar
return nil, fmt.Errorf("invalid state resulting from update: %w", err)
}

return &types.MsgUpdateMarketsResponse{}, ms.k.SetLastUpdated(ctx, uint64(ctx.BlockHeight())) //nolint:gosec
return &types.MsgUpdateMarketsResponse{}, nil
}

// verifyMarketAuthorities verifies that the msg-submitter is a market-authority
Expand Down
11 changes: 5 additions & 6 deletions x/marketmap/keeper/msg_server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,8 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"
authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
govtypes "github.com/cosmos/cosmos-sdk/x/gov/types"
"github.com/stretchr/testify/mock"

"github.com/skip-mev/chaintestutil/sample"
"github.com/stretchr/testify/mock"

connecttypes "github.com/skip-mev/connect/v2/pkg/types"
"github.com/skip-mev/connect/v2/x/marketmap/keeper"
Expand Down Expand Up @@ -394,7 +393,7 @@ func (s *KeeperTestSuite) TestMsgServerUpsertMarkets() {

hooks.On("AfterMarketCreated", mock.Anything, btcusdt).Return(nil).Once()

s.ctx = s.ctx.WithBlockHeight(12)
s.ctx = s.ctx.WithBlockHeight(r.Int63())

resp, err := msgServer.UpsertMarkets(s.ctx, msg)
s.Require().NoError(err)
Expand Down Expand Up @@ -436,7 +435,7 @@ func (s *KeeperTestSuite) TestMsgServerUpsertMarkets() {
hooks.On("AfterMarketCreated", mock.Anything, usdtusd).Return(nil).Once()
hooks.On("AfterMarketUpdated", mock.Anything, btcusdt).Return(err).Once()

s.ctx = s.ctx.WithBlockHeight(13)
s.ctx = s.ctx.WithBlockHeight(r.Int63())

resp, err := msgServer.UpsertMarkets(s.ctx, msg)
s.Require().Error(err)
Expand All @@ -457,7 +456,7 @@ func (s *KeeperTestSuite) TestMsgServerUpsertMarkets() {
hooks.On("AfterMarketUpdated", mock.Anything, btcusdt).Return(nil).Once()
hooks.On("AfterMarketCreated", mock.Anything, ethusdt).Return(nil).Once()

s.ctx = s.ctx.WithBlockHeight(13)
s.ctx = s.ctx.WithBlockHeight(r.Int63())

resp, err := msgServer.UpsertMarkets(s.ctx, msg)
s.Require().NoError(err)
Expand Down Expand Up @@ -512,7 +511,7 @@ func (s *KeeperTestSuite) TestMsgServerUpsertMarkets() {
}
hooks.On("AfterMarketUpdated", mock.Anything, mock.Anything).Return(nil).Once()

s.ctx = s.ctx.WithBlockHeight(13)
s.ctx = s.ctx.WithBlockHeight(r.Int63())

resp, err := msgServer.UpsertMarkets(s.ctx, msg)
s.Require().Error(err)
Expand Down
Loading