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

fix: fun token state marshalling #1947

Merged
merged 8 commits into from
Jun 28, 2024
Merged

fix: fun token state marshalling #1947

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0ca249b
fix: HexAddr marshalling issue
k-yang Jun 28, 2024
944d4c0
add hex addr string test
k-yang Jun 28, 2024
870e35c
refactor: remove EthAddr alias type
k-yang Jun 28, 2024
7378486
refactor: remove EthHash alias type
k-yang Jun 28, 2024
7e1e5b2
refactor: remove extra types
k-yang Jun 28, 2024
d2d745a
refactor: FunToken ID generation
k-yang Jun 28, 2024
325a2e4
fix: test fun token state
k-yang Jun 28, 2024
f3ac2f0
Update CHANGELOG.md
k-yang Jun 28, 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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- [#1917](https://github.com/NibiruChain/nibiru/pull/1917) - test(e2e-evm): TypeScript support. Type generation from compiled contracts. Formatter for TS code.
- [#1922](https://github.com/NibiruChain/nibiru/pull/1922) - feat(evm): tracer option is read from the config.
- [#1936](https://github.com/NibiruChain/nibiru/pull/1936) - feat(evm): EVM fungible token protobufs and encoding tests
- [#1947](https://github.com/NibiruChain/nibiru/pull/1947) - fix(evm): fix FunToken state marshalling
Copy link
Contributor

Choose a reason for hiding this comment

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

Enhance the changelog entry for better clarity.

The current changelog entry for PR 1947 is a bit vague. It would be beneficial for users and developers if the entry could provide a bit more context about what exactly was fixed with the FunToken state marshalling and why it was significant.

- [#1947](https://github.com/NibiruChain/nibiru/pull/1947) - fix(evm): Address an issue with `FunToken` state marshalling that previously led to incorrect state transitions under specific conditions.


#### Dapp modules: perp, spot, oracle, etc

Expand Down
6 changes: 3 additions & 3 deletions eth/hex.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

var _ sdk.CustomProtobufType = (*HexAddr)(nil)

func NewHexAddr(addr EthAddr) HexAddr {
func NewHexAddr(addr gethcommon.Address) HexAddr {
return HexAddr(addr.Hex())
}

Expand Down Expand Up @@ -58,7 +58,7 @@
return nil
}

func (h HexAddr) ToAddr() EthAddr {
func (h HexAddr) ToAddr() gethcommon.Address {
return gethcommon.HexToAddress(string(h))
}

Expand Down Expand Up @@ -88,7 +88,7 @@
// Implements the gogo proto custom type interface.
// Ref: https://github.com/cosmos/gogoproto/blob/v1.5.0/custom_types.md
func (h *HexAddr) MarshalTo(data []byte) (n int, err error) {
bz := []byte{}
bz := []byte(*h)

Check warning on line 91 in eth/hex.go

View check run for this annotation

Codecov / codecov/patch

eth/hex.go#L91 

Added line #L91 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

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

Potential issue in MarshalTo method.

The MarshalTo method seems to unnecessarily recreate the HexAddr from a string. This could be simplified and optimized.

-	hexAddr, err := NewHexAddrFromStr(string(bz))
-	*h = hexAddr
+	// Removed unnecessary conversion
+	err = nil

Additionally, this line was flagged by codecov as not covered by tests.

Would you like me to help add a test case for this method?

Committable suggestion was skipped due to low confidence.

Tools
GitHub Check: codecov/patch

[warning] 91-91: eth/hex.go#L91
Added line #L91 was not covered by tests

copy(data, bz)
hexAddr, err := NewHexAddrFromStr(string(bz))
*h = hexAddr
Expand Down
9 changes: 9 additions & 0 deletions eth/hex_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,3 +270,12 @@ func (s *Suite) TestProtobufEncoding() {
})
}
}

func (s *Suite) TestHexAddrToString() {
hexAddr := eth.HexAddr("0x5aAeb6053F3E94C9b9A09f33669435E7Ef1BeAed")
s.Equal("0x5aAeb6053F3E94C9b9A09f33669435E7Ef1BeAed", hexAddr.String())
s.Equal("0x5aAeb6053F3E94C9b9A09f33669435E7Ef1BeAed", string(hexAddr))

ethAddr := gethcommon.HexToAddress("0x5aAeb6053F3E94C9b9A09f33669435E7Ef1BeAed")
s.Equal("0x5aAeb6053F3E94C9b9A09f33669435E7Ef1BeAed", ethAddr.String())
}
32 changes: 13 additions & 19 deletions eth/state_encoder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,24 +12,18 @@
return fmt.Sprintf("%x", bz)
}

// EthAddr: (alias) 20 byte address of an Ethereum account.
type EthAddr = gethcommon.Address

// EthHash: (alias) 32 byte Keccak256 hash of arbitrary data.
type EthHash = gethcommon.Hash //revive:disable-line:exported

var (
// Implements a `collections.ValueEncoder` for the `[]byte` type
ValueEncoderBytes collections.ValueEncoder[[]byte] = veBytes{}
KeyEncoderBytes collections.KeyEncoder[[]byte] = keBytes{}

// Implements a `collections.ValueEncoder` for an Ethereum address.
ValueEncoderEthAddr collections.ValueEncoder[EthAddr] = veEthAddr{}
ValueEncoderEthAddr collections.ValueEncoder[gethcommon.Address] = veEthAddr{}
// keEthHash: Implements a `collections.KeyEncoder` for an Ethereum address.
KeyEncoderEthAddr collections.KeyEncoder[EthAddr] = keEthAddr{}
KeyEncoderEthAddr collections.KeyEncoder[gethcommon.Address] = keEthAddr{}

// keEthHash: Implements a `collections.KeyEncoder` for an Ethereum hash.
KeyEncoderEthHash collections.KeyEncoder[EthHash] = keEthHash{}
KeyEncoderEthHash collections.KeyEncoder[gethcommon.Hash] = keEthHash{}
)

// collections ValueEncoder[[]byte]
Expand All @@ -43,10 +37,10 @@
// veEthAddr: Implements a `collections.ValueEncoder` for an Ethereum address.
type veEthAddr struct{}

func (_ veEthAddr) Encode(value EthAddr) []byte { return value.Bytes() }
func (_ veEthAddr) Decode(bz []byte) EthAddr { return gethcommon.BytesToAddress(bz) }
func (_ veEthAddr) Stringify(value EthAddr) string { return value.Hex() }
func (_ veEthAddr) Name() string { return "EthAddr" }
func (_ veEthAddr) Encode(value gethcommon.Address) []byte { return value.Bytes() }
func (_ veEthAddr) Decode(bz []byte) gethcommon.Address { return gethcommon.BytesToAddress(bz) }
func (_ veEthAddr) Stringify(value gethcommon.Address) string { return value.Hex() }
func (_ veEthAddr) Name() string { return "gethcommon.Address" }

type keBytes struct{}

Expand All @@ -63,17 +57,17 @@
// keEthAddr: Implements a `collections.KeyEncoder` for an Ethereum address.
type keEthAddr struct{}

func (_ keEthAddr) Encode(value EthAddr) []byte { return value.Bytes() }
func (_ keEthAddr) Decode(bz []byte) (int, EthAddr) {
func (_ keEthAddr) Encode(value gethcommon.Address) []byte { return value.Bytes() }
func (_ keEthAddr) Decode(bz []byte) (int, gethcommon.Address) {
return gethcommon.AddressLength, gethcommon.BytesToAddress(bz)
}
func (_ keEthAddr) Stringify(value EthAddr) string { return value.Hex() }
func (_ keEthAddr) Stringify(value gethcommon.Address) string { return value.Hex() }

// keEthHash: Implements a `collections.KeyEncoder` for an Ethereum hash.
type keEthHash struct{}

func (_ keEthHash) Encode(value EthHash) []byte { return value.Bytes() }
func (_ keEthHash) Decode(bz []byte) (int, EthHash) {
func (_ keEthHash) Encode(value gethcommon.Hash) []byte { return value.Bytes() }
func (_ keEthHash) Decode(bz []byte) (int, gethcommon.Hash) {

Check warning on line 70 in eth/state_encoder.go

View check run for this annotation

Codecov / codecov/patch

eth/state_encoder.go#L69-L70 

Added lines #L69 - L70 were not covered by tests
return gethcommon.HashLength, gethcommon.BytesToHash(bz)
}
func (_ keEthHash) Stringify(value EthHash) string { return value.Hex() }
func (_ keEthHash) Stringify(value gethcommon.Hash) string { return value.Hex() }

Check warning on line 73 in eth/state_encoder.go

View check run for this annotation

Codecov / codecov/patch

eth/state_encoder.go#L73 

Added line #L73 was not covered by tests
4 changes: 2 additions & 2 deletions eth/state_encoder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ func (s *Suite) TestEncoderBytes() {
func (s *Suite) TestEncoderEthAddr() {
testCases := []struct {
name string
given eth.EthAddr
given gethcommon.Address
wantPanic bool
}{
{
Expand All @@ -77,7 +77,7 @@ func (s *Suite) TestEncoderEthAddr() {
},
{
name: "Nibiru Bech 32 addr (hypothetically)",
given: eth.EthAddr([]byte("nibi1rlvdjfmxkyfj4tzu73p8m4g2h4y89xccf9622l")),
given: gethcommon.Address([]byte("nibi1rlvdjfmxkyfj4tzu73p8m4g2h4y89xccf9622l")),
},
}
for _, tc := range testCases {
Expand Down
19 changes: 7 additions & 12 deletions x/evm/evm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,29 +15,24 @@
// tokens that are valid ERC20s with the same address.
// https://github.com/NibiruChain/nibiru/issues/1933
func (fun FunToken) ID() []byte {
return newFunTokenIDFromStr(fun.Erc20Addr.String(), fun.BankDenom)
return NewFunTokenID(fun.Erc20Addr, fun.BankDenom)

Check warning on line 18 in x/evm/evm.go

View check run for this annotation

Codecov / codecov/patch

x/evm/evm.go#L18 

Added line #L18 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip

Codebase Verification

Add test coverage for the ID method.

The method is crucial for generating unique identifiers but lacks test coverage. Ensure to include test cases that validate the correctness of the ID method in x/evm/evm.go.

  • Method: ID in x/evm/evm.go
Analysis chain

Add test coverage for the ID method.

The method is crucial for generating unique identifiers but lacks test coverage.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the test coverage for `ID` method in `x/evm/evm.go`.

# Test: Search for test cases covering the `ID` method. Expect: At least one test case.
rg --type python $'test.*ID' x/evm/evm_test.go

Length of output: 78

Script:

#!/bin/bash
# Description: Verify the test coverage for `ID` method in `x/evm/evm.go`.

# Test: Search for test cases covering the `ID` method. Expect: At least one test case.
rg --type go 'ID' x/evm/evm_test.go

Length of output: 35

Script:

#!/bin/bash
# Description: Verify the test coverage for `ID` method in `x/evm/evm.go`.

# Test: Search for test cases covering the `ID` method across all Go test files. Expect: At least one test case.
rg --type go 'ID' --glob '*.go'

Length of output: 57772

Tools
GitHub Check: codecov/patch

[warning] 18-18: x/evm/evm.go#L18
Added line #L18 was not covered by tests

}

func NewFunTokenID(erc20 gethcommon.Address, bankDenom string) []byte {
erc20Addr := erc20.Hex()
return newFunTokenIDFromStr(erc20Addr, bankDenom)
func NewFunTokenID(erc20 eth.HexAddr, bankDenom string) []byte {
return tmhash.Sum([]byte(erc20.String() + "|" + bankDenom))

Check warning on line 22 in x/evm/evm.go

View check run for this annotation

Codecov / codecov/patch

x/evm/evm.go#L21-L22 

Added lines #L21 - L22 were not covered by tests
}

func newFunTokenIDFromStr(erc20AddrHex string, bankDenom string) []byte {
return tmhash.Sum([]byte(erc20AddrHex + "|" + bankDenom))
}

func errValidateFunToken(errMsg string) error {
return fmt.Errorf("FunTokenError: %s", errMsg)
func funTokenValidationError(err error) error {
return fmt.Errorf("FunTokenError: %s", err.Error())
}

func (fun FunToken) Validate() error {
if err := sdk.ValidateDenom(fun.BankDenom); err != nil {
return errValidateFunToken(err.Error())
return funTokenValidationError(err)
}

if err := fun.Erc20Addr.Valid(); err != nil {
return errValidateFunToken(err.Error())
return funTokenValidationError(err)
}

return nil
Expand Down
4 changes: 2 additions & 2 deletions x/evm/keeper/evm_state.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ func (k Keeper) SetParams(ctx sdk.Context, params evm.Params) {

// SetState updates contract storage and deletes if the value is empty.
func (state EvmState) SetAccState(
ctx sdk.Context, addr eth.EthAddr, stateKey eth.EthHash, stateValue []byte,
ctx sdk.Context, addr gethcommon.Address, stateKey gethcommon.Hash, stateValue []byte,
) {
if len(stateValue) != 0 {
state.AccState.Insert(ctx, collections.Join(addr, stateKey), stateValue)
Expand All @@ -140,7 +140,7 @@ func (state EvmState) SetAccState(
}

// GetState: Implements `statedb.Keeper` interface: Loads smart contract state.
func (k *Keeper) GetState(ctx sdk.Context, addr eth.EthAddr, stateKey eth.EthHash) eth.EthHash {
func (k *Keeper) GetState(ctx sdk.Context, addr gethcommon.Address, stateKey gethcommon.Hash) gethcommon.Hash {
return gethcommon.BytesToHash(k.EvmState.AccState.GetOr(
ctx,
collections.Join(addr, stateKey),
Expand Down
18 changes: 6 additions & 12 deletions x/evm/keeper/funtoken_state.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,27 +10,21 @@ import (

"github.com/NibiruChain/nibiru/eth"
"github.com/NibiruChain/nibiru/x/evm"
funtoken "github.com/NibiruChain/nibiru/x/evm"
)

type (
funtokenPrimaryKeyType = []byte
Copy link
Member

Choose a reason for hiding this comment

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

I think the primary key type is easier to follow when reading the collections below than using []byte

funtokenValueType = funtoken.FunToken
)

// FunTokenState isolates the key-value stores (collections) for fungible token
// mappings. This struct is written as an extension of the default indexed map to
// add utility functions.
type FunTokenState struct {
collections.IndexedMap[funtokenPrimaryKeyType, funtokenValueType, IndexesFunToken]
collections.IndexedMap[[]byte, evm.FunToken, IndexesFunToken]
}

func NewFunTokenState(
cdc sdkcodec.BinaryCodec,
storeKey storetypes.StoreKey,
) FunTokenState {
primaryKeyEncoder := eth.KeyEncoderBytes
valueEncoder := collections.ProtoValueEncoder[funtokenValueType](cdc)
valueEncoder := collections.ProtoValueEncoder[evm.FunToken](cdc)
idxMap := collections.NewIndexedMap(
storeKey, evm.KeyPrefixFunTokens, primaryKeyEncoder, valueEncoder,
IndexesFunToken{
Expand All @@ -53,8 +47,8 @@ func NewFunTokenState(
return FunTokenState{IndexedMap: idxMap}
}

func (idxs IndexesFunToken) IndexerList() []collections.Indexer[funtokenPrimaryKeyType, funtokenValueType] {
return []collections.Indexer[funtokenPrimaryKeyType, funtokenValueType]{
func (idxs IndexesFunToken) IndexerList() []collections.Indexer[[]byte, evm.FunToken] {
return []collections.Indexer[[]byte, evm.FunToken]{
idxs.ERC20Addr,
idxs.BankDenom,
}
Expand All @@ -66,13 +60,13 @@ type IndexesFunToken struct {
// - indexing key (IK): ERC-20 addr
// - primary key (PK): FunToken ID
// - value (V): FunToken value
ERC20Addr collections.MultiIndex[gethcommon.Address, funtokenPrimaryKeyType, funtokenValueType]
ERC20Addr collections.MultiIndex[gethcommon.Address, []byte, evm.FunToken]

// BankDenom (MultiIndex): Index FunToken by coin denomination
// - indexing key (IK): Coin denom
// - primary key (PK): FunToken ID
// - value (V): FunToken value
BankDenom collections.MultiIndex[string, funtokenPrimaryKeyType, funtokenValueType]
BankDenom collections.MultiIndex[string, []byte, evm.FunToken]
}

// Insert adds an [evm.FunToken] to state with defensive validation. Errors if
Expand Down
82 changes: 82 additions & 0 deletions x/evm/keeper/funtoken_state_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
package keeper_test

import (
gethcommon "github.com/ethereum/go-ethereum/common"

"github.com/NibiruChain/nibiru/eth"
"github.com/NibiruChain/nibiru/x/evm"
"github.com/NibiruChain/nibiru/x/evm/evmtest"
)

func (s *KeeperSuite) TestInsertAndGet() {
deps := evmtest.NewTestDeps()

erc20Addr := gethcommon.HexToAddress("0xAEf9437FF23D48D73271a41a8A094DEc9ac71477")
err := deps.K.FunTokens.SafeInsert(
deps.Ctx,
erc20Addr,
"unibi",
true,
)
s.Require().NoError(err)

// test Get
funToken, err := deps.K.FunTokens.Get(deps.Ctx, evm.NewFunTokenID(eth.NewHexAddr(erc20Addr), "unibi"))
s.Require().NoError(err)
s.Require().Equal(eth.HexAddr("0xAEf9437FF23D48D73271a41a8A094DEc9ac71477"), funToken.Erc20Addr)
s.Require().Equal("unibi", funToken.BankDenom)
s.Require().True(funToken.IsMadeFromCoin)

// iter := deps.K.FunTokens.Indexes.BankDenom.ExactMatch(deps.Ctx, "unibi")
// deps.K.FunTokens.Collect(ctx)
}

func (s *KeeperSuite) TestCollect() {
deps := evmtest.NewTestDeps()

erc20Addr := gethcommon.HexToAddress("0xAEf9437FF23D48D73271a41a8A094DEc9ac71477")
err := deps.K.FunTokens.SafeInsert(
deps.Ctx,
erc20Addr,
"unibi",
true,
)
s.Require().NoError(err)

// test Collect by bank denom
iter := deps.K.FunTokens.Indexes.BankDenom.ExactMatch(deps.Ctx, "unibi")
funTokens := deps.K.FunTokens.Collect(deps.Ctx, iter)
s.Require().Len(funTokens, 1)
s.Require().Equal(eth.HexAddr("0xAEf9437FF23D48D73271a41a8A094DEc9ac71477"), funTokens[0].Erc20Addr)
s.Require().Equal("unibi", funTokens[0].BankDenom)
s.Require().True(funTokens[0].IsMadeFromCoin)

// test Collect by erc20 addr
iter2 := deps.K.FunTokens.Indexes.ERC20Addr.ExactMatch(deps.Ctx, erc20Addr)
funTokens = deps.K.FunTokens.Collect(deps.Ctx, iter2)
s.Require().Len(funTokens, 1)
s.Require().Equal(eth.HexAddr("0xAEf9437FF23D48D73271a41a8A094DEc9ac71477"), funTokens[0].Erc20Addr)
s.Require().Equal("unibi", funTokens[0].BankDenom)
s.Require().True(funTokens[0].IsMadeFromCoin)
}

func (s *KeeperSuite) TestDelete() {
deps := evmtest.NewTestDeps()

erc20Addr := gethcommon.HexToAddress("0xAEf9437FF23D48D73271a41a8A094DEc9ac71477")
err := deps.K.FunTokens.SafeInsert(
deps.Ctx,
erc20Addr,
"unibi",
true,
)
s.Require().NoError(err)

// test Delete
err = deps.K.FunTokens.Delete(deps.Ctx, evm.NewFunTokenID(eth.NewHexAddr(erc20Addr), "unibi"))
s.Require().NoError(err)

// test Get
_, err = deps.K.FunTokens.Get(deps.Ctx, evm.NewFunTokenID(eth.NewHexAddr(erc20Addr), "unibi"))
s.Require().Error(err)
}
Loading