fix: wrong denom check at RegisterCoin

problem: - `coinMetadata.Base` is used as the denom for the token pair - However, the check is incorrectly performed using `coinMetadata.Name`, so it fails to detect if the denom is already registered solution: check using `coinMetadata.Base`, not `.Name` more context: Unfortunately, there is already one case on the mainnet where multiple token pairs exist for the same denom(= ibc/13B6057538B93225F6EBACCB64574C49B2C1568C5AE6CCFE0A039D7DAC02BF29). However, this is not a major issue because only the latest token pair registered in the denom index is actually used. With this patch, such cases will no longer occur.
b-harvest · Jul 25, 2024 · d01eab0 · d01eab0
1 parent d4c9382
commit d01eab0
Show file tree

Hide file tree

Showing 3 changed files with 66 additions and 48 deletions.
diff --git a/x/erc20/keeper/evm_hooks_test.go b/x/erc20/keeper/evm_hooks_test.go
@@ -17,15 +17,14 @@ import (
 )
 
 const (
-	erc20Name          = "Coin Token"
-	erc20Symbol        = "CTKN"
-	erc20Decimals      = uint8(18)
-	cosmosTokenBase    = "acoin"
-	cosmosTokenDisplay = "coin"
-	cosmosDecimals     = uint8(6)
-	defaultExponent    = uint32(18)
-	zeroExponent       = uint32(0)
-	ibcBase            = "ibc/7F1D3FCF4AE79E1554D670D1AD949A9BA4E4A3C76C63093E17E446A46061A7A2"
+	erc20Name       = "Coin Token"
+	erc20Symbol     = "CTKN"
+	erc20Decimals   = uint8(18)
+	cosmosTokenBase = "acoin"
+	cosmosDecimals  = uint8(6)
+	defaultExponent = uint32(18)
+	zeroExponent    = uint32(0)
+	ibcBase         = "ibc/7F1D3FCF4AE79E1554D670D1AD949A9BA4E4A3C76C63093E17E446A46061A7A2"
 )
 
 func (suite *KeeperTestSuite) setupRegisterCoin() (banktypes.Metadata, *types.TokenPair) {

diff --git a/x/erc20/keeper/proposals.go b/x/erc20/keeper/proposals.go
@@ -34,7 +34,7 @@ func (k Keeper) RegisterCoin(
 	}
 
 	// Check if denomination is already registered
-	if k.IsDenomRegistered(ctx, coinMetadata.Name) {
+	if k.IsDenomRegistered(ctx, coinMetadata.Base) {
 		return nil, errorsmod.Wrapf(
 			types.ErrTokenPairAlreadyExists, "coin denomination already registered: %s", coinMetadata.Name,
 		)

diff --git a/x/erc20/keeper/proposals_test.go b/x/erc20/keeper/proposals_test.go
@@ -20,24 +20,32 @@ import (
 	inflationtypes "github.com/Canto-Network/Canto/v7/x/inflation/types"
 )
 
+// The following constants are based on query result of mainnet denoms metadata query.
+const (
+	gravitonIBCDenom = "ibc/FC9D92EC12BC974E8B6179D411351524CD5C2EBC3CE29D5BA856414FEFA47093"
+	gravitonDenom    = "graviton"
+	gravitonName     = "Graviton"
+	gravitonSymbol   = "GRAV"
+)
+
 func (suite KeeperTestSuite) TestRegisterCoin() {
 	metadata := banktypes.Metadata{
 		Description: "description",
-		Base:        cosmosTokenBase,
+		Base:        gravitonIBCDenom,
 		// NOTE: Denom units MUST be increasing
 		DenomUnits: []*banktypes.DenomUnit{
 			{
-				Denom:    cosmosTokenBase,
+				Denom:    gravitonIBCDenom,
 				Exponent: 0,
 			},
 			{
-				Denom:    cosmosTokenDisplay,
+				Denom:    gravitonDenom,
 				Exponent: defaultExponent,
 			},
 		},
-		Name:    cosmosTokenBase,
-		Symbol:  erc20Symbol,
-		Display: cosmosTokenDisplay,
+		Name:    gravitonName,
+		Symbol:  gravitonSymbol,
+		Display: gravitonDenom,
 	}
 
 	testCases := []struct {
@@ -46,7 +54,7 @@ func (suite KeeperTestSuite) TestRegisterCoin() {
 		expPass  bool
 	}{
 		{
-			"conversion is disabled globally",
+			"fail: conversion is disabled globally",
 			func() {
 				params := types.DefaultParams()
 				params.EnableErc20 = false
@@ -55,41 +63,42 @@ func (suite KeeperTestSuite) TestRegisterCoin() {
 			false,
 		},
 		{
-			"denom already registered",
+			"fail: denom already registered",
 			func() {
 				regPair := types.NewTokenPair(tests.GenerateAddress(), metadata.Base, true, types.OWNER_MODULE)
+				suite.app.Erc20Keeper.SetTokenPair(suite.ctx, regPair)
 				suite.app.Erc20Keeper.SetTokenPairIdByDenom(suite.ctx, regPair.Denom, regPair.GetID())
 				suite.Commit()
 			},
 			false,
 		},
 		{
-			"token doesn't have supply",
+			"fail :token doesn't have supply",
 			func() {
 			},
 			false,
 		},
 		{
-			"metadata different that stored",
+			"fail: metadata different that stored",
 			func() {
-				metadata.Base = cosmosTokenBase
+				metadata.Base = gravitonDenom
 				validMetadata := banktypes.Metadata{
 					Description: "description",
-					Base:        cosmosTokenBase,
+					Base:        gravitonIBCDenom,
 					// NOTE: Denom units MUST be increasing
 					DenomUnits: []*banktypes.DenomUnit{
 						{
-							Denom:    cosmosTokenBase,
+							Denom:    gravitonIBCDenom,
 							Exponent: 0,
 						},
 						{
-							Denom:    cosmosTokenDisplay,
-							Exponent: uint32(18),
+							Denom:    gravitonDenom,
+							Exponent: defaultExponent,
 						},
 					},
-					Name:    erc20Name,
-					Symbol:  erc20Symbol,
-					Display: cosmosTokenDisplay,
+					Name:    "different name",
+					Symbol:  gravitonSymbol,
+					Display: gravitonDenom,
 				}
 
 				err := suite.app.BankKeeper.MintCoins(suite.ctx, inflationtypes.ModuleName, sdk.Coins{sdk.NewInt64Coin(validMetadata.Base, 1)})
@@ -108,16 +117,7 @@ func (suite KeeperTestSuite) TestRegisterCoin() {
 			false,
 		},
 		{
-			"evm denom registration - CANTO",
-			func() {
-				metadata.Base = "CANTO"
-				err := suite.app.BankKeeper.MintCoins(suite.ctx, inflationtypes.ModuleName, sdk.Coins{sdk.NewInt64Coin(metadata.Base, 1)})
-				suite.Require().NoError(err)
-			},
-			false,
-		},
-		{
-			"evm denom registration - aCANTO",
+			"fail: evm denom registration - aCANTO",
 			func() {
 				metadata.Base = "aCANTO"
 				err := suite.app.BankKeeper.MintCoins(suite.ctx, inflationtypes.ModuleName, sdk.Coins{sdk.NewInt64Coin(metadata.Base, 1)})
@@ -126,7 +126,7 @@ func (suite KeeperTestSuite) TestRegisterCoin() {
 			false,
 		},
 		{
-			"evm denom registration - wCANTO",
+			"fail: evm denom registration - wCANTO",
 			func() {
 				metadata.Base = "wCANTO"
 				err := suite.app.BankKeeper.MintCoins(suite.ctx, inflationtypes.ModuleName, sdk.Coins{sdk.NewInt64Coin(metadata.Base, 1)})
@@ -137,16 +137,16 @@ func (suite KeeperTestSuite) TestRegisterCoin() {
 		{
 			"ok",
 			func() {
-				metadata.Base = cosmosTokenBase
+				metadata.Base = gravitonIBCDenom
 				err := suite.app.BankKeeper.MintCoins(suite.ctx, inflationtypes.ModuleName, sdk.Coins{sdk.NewInt64Coin(metadata.Base, 1)})
 				suite.Require().NoError(err)
 			},
 			true,
 		},
 		{
-			"force fail evm",
+			"fail: force fail evm",
 			func() {
-				metadata.Base = cosmosTokenBase
+				metadata.Base = gravitonDenom
 				err := suite.app.BankKeeper.MintCoins(suite.ctx, inflationtypes.ModuleName, sdk.Coins{sdk.NewInt64Coin(metadata.Base, 1)})
 				suite.Require().NoError(err)
 
@@ -160,14 +160,33 @@ func (suite KeeperTestSuite) TestRegisterCoin() {
 			false,
 		},
 		{
-			"force delete module account evm",
+			"fail: force delete module account evm",
+			func() {
+				metadata.Base = gravitonIBCDenom
+				err := suite.app.BankKeeper.MintCoins(suite.ctx, inflationtypes.ModuleName, sdk.Coins{sdk.NewInt64Coin(metadata.Base, 1)})
+				suite.Require().NoError(err)
+			},
+			false,
+		},
+		{
+			"fail: token pair already exists with same denom",
 			func() {
-				metadata.Base = cosmosTokenBase
+				metadata.Base = gravitonIBCDenom
 				err := suite.app.BankKeeper.MintCoins(suite.ctx, inflationtypes.ModuleName, sdk.Coins{sdk.NewInt64Coin(metadata.Base, 1)})
 				suite.Require().NoError(err)
 
-				acc := suite.app.AccountKeeper.GetAccount(suite.ctx, types.ModuleAddress.Bytes())
-				suite.app.AccountKeeper.RemoveAccount(suite.ctx, acc)
+				tokenPair, err := suite.app.Erc20Keeper.RegisterCoin(suite.ctx, metadata)
+				suite.Require().NoError(err)
+
+				// check token pair is stored
+				suite.Require().Equal(types.OWNER_MODULE, tokenPair.ContractOwner)
+				suite.Require().Equal(metadata.Base, tokenPair.Denom)
+				suite.Require().Equal(true, tokenPair.Enabled)
+
+				// check indexes are stored
+				id := tokenPair.GetID()
+				suite.Require().Equal(id, suite.app.Erc20Keeper.GetTokenPairIdByDenom(suite.ctx, tokenPair.Denom))
+				suite.Require().Equal(id, suite.app.Erc20Keeper.GetTokenPairIdByERC20Addr(suite.ctx, common.HexToAddress(tokenPair.Erc20Address)))
 			},
 			false,
 		},
@@ -183,9 +202,9 @@ func (suite KeeperTestSuite) TestRegisterCoin() {
 
 			expPair := &types.TokenPair{
 				Erc20Address:  "0x80b5a32E4F032B2a058b4F29EC95EEfEEB87aDcd",
-				Denom:         "acoin",
+				Denom:         gravitonIBCDenom,
 				Enabled:       true,
-				ContractOwner: 1,
+				ContractOwner: types.OWNER_MODULE,
 			}
 
 			if tc.expPass {