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.

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

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
3 changes: 2 additions & 1 deletion tests/integration/connect_ccv_suite.go
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ func (s *ConnectCCVSuite) TestCCVAggregation() {
}

// update the market
s.UpdateCurrencyPair(s.chain, []mmtypes.Market{
err = s.UpdateCurrencyPair(ctx, s.chain, []mmtypes.Market{
{
Ticker: mmtypes.Ticker{
CurrencyPair: ethusdc,
Expand All @@ -187,6 +187,7 @@ func (s *ConnectCCVSuite) TestCCVAggregation() {
},
},
})
s.Require().NoError(err)

priceDelta := big.NewInt(-60000000000)
bz, err := priceDelta.GobEncode()
Expand Down
14 changes: 5 additions & 9 deletions tests/integration/connect_setup.go
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,7 @@ func QueryCurrencyPair(chain *cosmos.CosmosChain, cp connecttypes.CurrencyPair,
}

// QueryMarket queries a market from the market map.
func QueryMarket(chain *cosmos.CosmosChain, cp connecttypes.CurrencyPair) (mmtypes.Market, error) {
func QueryMarket(ctx context.Context, chain *cosmos.CosmosChain, cp connecttypes.CurrencyPair) (mmtypes.Market, error) {
grpcAddr := chain.GetHostGRPCAddress()

// create the client
Expand All @@ -319,8 +319,6 @@ func QueryMarket(chain *cosmos.CosmosChain, cp connecttypes.CurrencyPair) (mmtyp
// create the mm client
client := mmtypes.NewQueryClient(cc)

ctx := context.Background()

// query the currency pairs
res, err := client.Market(ctx, &mmtypes.MarketRequest{
CurrencyPair: cp,
Expand All @@ -333,7 +331,7 @@ func QueryMarket(chain *cosmos.CosmosChain, cp connecttypes.CurrencyPair) (mmtyp
}

// QueryMarketMap queries the market map.
func QueryMarketMap(chain *cosmos.CosmosChain) (*mmtypes.MarketMapResponse, error) {
func QueryMarketMap(ctx context.Context, chain *cosmos.CosmosChain) (*mmtypes.MarketMapResponse, error) {
grpcAddr := chain.GetHostGRPCAddress()

// create the client
Expand All @@ -346,8 +344,6 @@ func QueryMarketMap(chain *cosmos.CosmosChain) (*mmtypes.MarketMapResponse, erro
// create the mm client
client := mmtypes.NewQueryClient(cc)

ctx := context.Background()

// query the currency pairs
res, err := client.MarketMap(ctx, &mmtypes.MarketMapRequest{})
if err != nil {
Expand Down Expand Up @@ -455,7 +451,7 @@ func (s *ConnectIntegrationSuite) AddCurrencyPairs(chain *cosmos.CosmosChain, us
}

// check market map and lastUpdated
mmResp, err := QueryMarketMap(chain)
mmResp, err := QueryMarketMap(ctx, chain)
s.Require().NoError(err)

// ensure that the market exists
Expand Down Expand Up @@ -501,7 +497,7 @@ func (s *ConnectIntegrationSuite) RemoveMarket(
return nil
}

func (s *ConnectIntegrationSuite) UpdateCurrencyPair(chain *cosmos.CosmosChain, markets []mmtypes.Market) error {
func (s *ConnectIntegrationSuite) UpdateCurrencyPair(ctx context.Context, chain *cosmos.CosmosChain, markets []mmtypes.Market) error {
msg := &mmtypes.MsgUpsertMarkets{
Authority: s.user.FormattedAddress(),
Markets: markets,
Expand All @@ -522,7 +518,7 @@ func (s *ConnectIntegrationSuite) UpdateCurrencyPair(chain *cosmos.CosmosChain,
}

// check market map and lastUpdated
mmResp, err := QueryMarketMap(chain)
mmResp, err := QueryMarketMap(ctx, chain)
s.Require().NoError(err)

// ensure that the market exists
Expand Down
10 changes: 6 additions & 4 deletions tests/integration/connect_suite.go
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,8 @@ func NewConnectOracleIntegrationSuite(suite *ConnectIntegrationSuite) *ConnectOr
}

func (s *ConnectOracleIntegrationSuite) TestOracleModule() {
ctx := context.Background()

// query the oracle module grpc service for any CurrencyPairs
s.Run("QueryCurrencyPairs - no currency-pairs reported", func() {
resp, err := QueryCurrencyPairs(s.chain)
Expand Down Expand Up @@ -377,7 +379,7 @@ func (s *ConnectOracleIntegrationSuite) TestOracleModule() {
s.Require().Error(s.RemoveMarket(s.chain, []connecttypes.CurrencyPair{cp1}))

// check not removed
market, err := QueryMarket(s.chain, cp1)
market, err := QueryMarket(ctx, s.chain, cp1)
s.Require().NoError(err)
s.Require().NotNil(market)
})
Expand All @@ -389,22 +391,22 @@ func (s *ConnectOracleIntegrationSuite) TestOracleModule() {
disabledTicker(disabledCP),
}...))

market, err := QueryMarket(s.chain, disabledCP)
market, err := QueryMarket(ctx, s.chain, disabledCP)
s.Require().NoError(err)
s.Require().NotNil(market)

s.Require().NoError(s.RemoveMarket(s.chain, []connecttypes.CurrencyPair{disabledCP}))

// check removed
_, err = QueryMarket(s.chain, disabledCP)
_, err = QueryMarket(ctx, s.chain, disabledCP)
s.Require().Error(err)
})

s.Run("remove a non existent market", func() {
nonexistentCP := connecttypes.NewCurrencyPair("NON", "EXIST")

// check removed doesnt exist
_, err := QueryMarket(s.chain, nonexistentCP)
_, err := QueryMarket(ctx, s.chain, nonexistentCP)
s.Require().Error(err)

// tx will not error
Expand Down
15 changes: 13 additions & 2 deletions x/marketmap/keeper/keeper.go
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
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
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
Loading
Loading