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

refactor(types)!: removed global config for verifier #18607

Merged
merged 61 commits into from
Feb 2, 2024
Merged
Show file tree
Hide file tree
Changes from 52 commits
Commits
Show all changes
61 commits
Select commit Hold shift + click to select a range
5445533
moved cointype out of config
bizk Nov 6, 2023
bbb175c
small rollback
bizk Nov 8, 2023
42213bc
moved address config to address file
bizk Nov 15, 2023
99deec0
moved out purpose to address global config
bizk Nov 15, 2023
27e62ed
moving configs out of global to context
bizk Nov 21, 2023
f190645
Resolved address config issue
bizk Nov 23, 2023
272083d
solved lint issues
bizk Nov 23, 2023
a3070eb
fixed lint
bizk Nov 23, 2023
016259c
rolled back unnecesary change
bizk Nov 23, 2023
28344a3
Merge branch 'main' into feature/sdkConfig
bizk Nov 23, 2023
ae0dbc8
removed getCoinType
bizk Nov 27, 2023
2cdd14c
Merge branch 'feature/sdkConfig' of github.com:Zondax/cosmos-sdk into…
bizk Nov 27, 2023
e3bd0d2
removed pupose from config
bizk Nov 28, 2023
ce6e6e8
Merge branch 'main' into feature/sdkConfig
bizk Nov 28, 2023
999633c
removed setPurpose
bizk Nov 28, 2023
04b2a7a
Merge branch 'feature/sdkConfig' of github.com:Zondax/cosmos-sdk into…
bizk Nov 28, 2023
545c053
removed purpose and addressconfig
bizk Nov 28, 2023
d906067
removed missing fmt
bizk Nov 28, 2023
e5d6193
Merge branch 'main' into feature/sdkConfig
bizk Nov 28, 2023
246aedb
added changelog
bizk Nov 28, 2023
21d6c7d
improved changelog entry
bizk Nov 29, 2023
e21d6c0
removed GetFullFundraiserPath
bizk Nov 29, 2023
13d1ce9
removed addressVerifier global config usage
bizk Nov 30, 2023
b5138da
added missing changes
bizk Nov 30, 2023
de01090
separated custom address verification from bech32
bizk Dec 1, 2023
88ef673
Merge branch 'main' into feature/sdkConfig-verifier
bizk Dec 1, 2023
0f210e3
added changelog
bizk Dec 1, 2023
4f78965
moved addres codec bech32 verification to types/bech32
bizk Dec 6, 2023
1d30858
fixed linting errors
bizk Dec 6, 2023
a4f22ac
Merge branch 'main' of github.com:Zondax/cosmos-sdk into feature/sdkC…
bizk Dec 7, 2023
65bfcc3
Merge branch 'main' of github.com:Zondax/cosmos-sdk into feature/sdkC…
bizk Dec 7, 2023
a4ea39c
eliminated verifier
bizk Dec 7, 2023
a3b5ba1
Merge branch 'main' into feature/sdkConfig-verifier
bizk Dec 7, 2023
33a826d
fixed linting issues
bizk Dec 7, 2023
0b45fd6
Merge branch 'feature/sdkConfig-verifier' of github.com:Zondax/cosmos…
bizk Dec 7, 2023
84e8efb
Merge branch 'main' into feature/sdkConfig-verifier
bizk Dec 8, 2023
47b0bc2
fix lint
bizk Dec 8, 2023
e6c304c
Merge branch 'feature/sdkConfig-verifier' of github.com:Zondax/cosmos…
bizk Dec 8, 2023
c59f4b1
added check to codec
bizk Dec 8, 2023
17b77b9
Merge branch 'main' into feature/sdkConfig-verifier
bizk Dec 8, 2023
1094070
gci fix
bizk Dec 8, 2023
6f6c668
fixed comments and suggestions
bizk Dec 8, 2023
56beacc
fixed changelog.md comments
bizk Dec 11, 2023
1a0272e
Merge branch 'main' into feature/sdkConfig-verifier
bizk Dec 12, 2023
11adeae
Merge branch 'feature/sdkConfig-verifier' of github.com:Zondax/cosmos…
bizk Dec 19, 2023
26f6ad2
fixed comments
bizk Dec 19, 2023
333a0d1
Merge branch 'main' into feature/sdkConfig-verifier
bizk Dec 19, 2023
696d40f
removed deprecated tests
bizk Dec 19, 2023
37b83f6
added back address verificatiopn as part of bech32 codec
bizk Dec 20, 2023
bffaa0e
Merge branch 'main' into feature/sdkConfig-verifier
bizk Dec 20, 2023
71f9d5a
fixed comment
bizk Dec 21, 2023
5c6e148
Merge branch 'feature/sdkConfig-verifier' of github.com:Zondax/cosmos…
bizk Dec 21, 2023
f7f2691
ran lint fix
bizk Dec 21, 2023
1db671b
ran lint fix
bizk Dec 21, 2023
ec7daa8
Merge branch 'main' into feature/sdkConfig-verifier
bizk Dec 26, 2023
9d3c7e9
added codec
bizk Jan 24, 2024
d15a92e
update: ValAddressFromBech32 AccAddressFromBech32
JulianToledano Jan 30, 2024
ae6fc3b
Merge branch 'main' into feature/sdkConfig-verifier
JulianToledano Jan 31, 2024
d9437c5
Merge branch 'main' into feature/sdkConfig-verifier
JulianToledano Jan 31, 2024
83dc83b
update: ConsAddressFromBech32
JulianToledano Jan 31, 2024
4d8bf97
Merge branch 'main' into feature/sdkConfig-verifier
JulianToledano Feb 2, 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
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (x/auth) [#18351](https://github.com/cosmos/cosmos-sdk/pull/18351) Auth module was moved to its own go.mod `cosmossdk.io/x/auth`
* (types) [#18372](https://github.com/cosmos/cosmos-sdk/pull/18372) Removed global configuration for coin type and purpose. Setters and getters should be removed and access directly to defined types.
* (types) [#18695](https://github.com/cosmos/cosmos-sdk/pull/18695) Removed global configuration for txEncoder.
* (types) [#18607](https://github.com/cosmos/cosmos-sdk/pull/18607) Removed address verifier from global config, moved verifier function to bech32 codec.
bizk marked this conversation as resolved.
Show resolved Hide resolved

### CLI Breaking Changes

Expand Down
14 changes: 9 additions & 5 deletions codec/address/bech32_codec.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@ package address

import (
"errors"
sdkAddress "github.com/cosmos/cosmos-sdk/types/address"
"strings"

"cosmossdk.io/core/address"
errorsmod "cosmossdk.io/errors"

sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/bech32"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
)
Expand All @@ -33,12 +33,12 @@ func (bc Bech32Codec) StringToBytes(text string) ([]byte, error) {
return nil, err
}

if hrp != bc.Bech32Prefix {
return nil, errorsmod.Wrapf(sdkerrors.ErrLogic, "hrp does not match bech32 prefix: expected '%s' got '%s'", bc.Bech32Prefix, hrp)
if len(bz) > sdkAddress.MaxAddrLen {
return nil, errorsmod.Wrapf(sdkerrors.ErrUnknownAddress, "address max length is %d, got %d", sdkAddress.MaxAddrLen, len(bz))
}

if err := sdk.VerifyAddressFormat(bz); err != nil {
julienrbrt marked this conversation as resolved.
Show resolved Hide resolved
return nil, err
if hrp != bc.Bech32Prefix {
return nil, errorsmod.Wrapf(sdkerrors.ErrLogic, "hrp does not match bech32 prefix: expected '%s' got '%s'", bc.Bech32Prefix, hrp)
bizk marked this conversation as resolved.
Show resolved Hide resolved
}

return bz, nil
Expand All @@ -55,5 +55,9 @@ func (bc Bech32Codec) BytesToString(bz []byte) (string, error) {
return "", err
}

if len(bz) > sdkAddress.MaxAddrLen {
return "", errorsmod.Wrapf(sdkerrors.ErrUnknownAddress, "address max length is %d, got %d", sdkAddress.MaxAddrLen, len(bz))
}

return text, nil
}
1 change: 0 additions & 1 deletion fuzz/oss-fuzz-build.sh
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ build_go_fuzzer FuzzTendermintAminoDecodeTime fuzz_tendermint_amino_decodetime
build_go_fuzzer FuzzTypesParseCoin fuzz_types_parsecoin
build_go_fuzzer FuzzTypesParseDecCoin fuzz_types_parsedeccoin
build_go_fuzzer FuzzTypesParseTimeBytes fuzz_types_parsetimebytes
build_go_fuzzer FuzzTypesVerifyAddressFormat fuzz_types_verifyaddressformat
build_go_fuzzer FuzzTypesDecSetString fuzz_types_dec_setstring

build_go_fuzzer FuzzUnknownProto fuzz_unknownproto
15 changes: 0 additions & 15 deletions fuzz/tests/types_verifyaddressformat_test.go

This file was deleted.

41 changes: 0 additions & 41 deletions types/address.go
bizk marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,9 @@ import (
"github.com/hashicorp/golang-lru/simplelru"
"sigs.k8s.io/yaml"

errorsmod "cosmossdk.io/errors"

cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
"github.com/cosmos/cosmos-sdk/internal/conv"
"github.com/cosmos/cosmos-sdk/types/address"
"github.com/cosmos/cosmos-sdk/types/bech32"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
)
bizk marked this conversation as resolved.
Show resolved Hide resolved

const (
Expand Down Expand Up @@ -157,28 +153,6 @@ func AccAddressFromHexUnsafe(address string) (addr AccAddress, err error) {
return AccAddress(bz), err
}
bizk marked this conversation as resolved.
Show resolved Hide resolved

// VerifyAddressFormat verifies that the provided bytes form a valid address
// according to the default address rules or a custom address verifier set by
// GetConfig().SetAddressVerifier().
// TODO make an issue to get rid of global Config
// ref: https://github.com/cosmos/cosmos-sdk/issues/9690
func VerifyAddressFormat(bz []byte) error {
verifier := GetConfig().GetAddressVerifier()
if verifier != nil {
return verifier(bz)
}
Comment on lines -166 to -169
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 we need to keep this function and the verification in other address functions as not everyone will migrate right away. We would remove functionality to set custom verifiers but verify the basic we do now

Copy link
Member

Choose a reason for hiding this comment

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

People can use an address codec to do so tbh. It is being populated everywhere in v0.51 (clientCtx, sign mode context, keepers).
Keeping it around could lead to a divergence of the address codec verification and the one in the global config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do see a reason to have a verification function as @tac0turtle says, but I also think @julienrbrt is right I think it is redundant that users could send their own verification function. Maybe adding the verification function to the Address interface would solve the issue


if len(bz) == 0 {
return errorsmod.Wrap(sdkerrors.ErrUnknownAddress, "addresses cannot be empty")
}

if len(bz) > address.MaxAddrLen {
return errorsmod.Wrapf(sdkerrors.ErrUnknownAddress, "address max length is %d, got %d", address.MaxAddrLen, len(bz))
}

return nil
}

// MustAccAddressFromBech32 calls AccAddressFromBech32 and panics on error.
func MustAccAddressFromBech32(address string) AccAddress {
julienrbrt marked this conversation as resolved.
Show resolved Hide resolved
addr, err := AccAddressFromBech32(address)
bizk marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -202,11 +176,6 @@ func AccAddressFromBech32(address string) (addr AccAddress, err error) {
return nil, err
}

err = VerifyAddressFormat(bz)
julienrbrt marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return nil, err
}

return AccAddress(bz), nil
}

Expand Down Expand Up @@ -354,11 +323,6 @@ func ValAddressFromBech32(address string) (addr ValAddress, err error) {
return nil, err
}

err = VerifyAddressFormat(bz)
if err != nil {
return nil, err
}

return ValAddress(bz), nil
bizk marked this conversation as resolved.
Show resolved Hide resolved
}

Expand Down Expand Up @@ -519,11 +483,6 @@ func ConsAddressFromBech32(address string) (addr ConsAddress, err error) {
return nil, err
}

err = VerifyAddressFormat(bz)
JulianToledano marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return nil, err
}

return ConsAddress(bz), nil
Copy link
Contributor

Choose a reason for hiding this comment

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

The ConsAddressFromBech32 function's error handling has been updated similarly to AccAddressFromBech32. Ensure that the new address verification logic is also applied here.

func ConsAddressFromBech32(address string) (addr ConsAddress, err error) {
  // ... existing logic ...
  // Add call to new verifyAddress function
  if err := verifyAddress(bz); err != nil {
    return nil, err
  }
  // ... existing logic ...
}

}

Expand Down
61 changes: 0 additions & 61 deletions types/address_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"bytes"
"crypto/rand"
"encoding/hex"
"fmt"
mathrand "math/rand"
"strings"
"testing"
Expand Down Expand Up @@ -381,66 +380,6 @@ func (s *addressTestSuite) TestAddressInterface() {
}
}

func (s *addressTestSuite) TestVerifyAddressFormat() {
addr0 := make([]byte, 0)
addr5 := make([]byte, 5)
addr20 := make([]byte, 20)
addr32 := make([]byte, 32)
addr256 := make([]byte, 256)

err := types.VerifyAddressFormat(addr0)
s.Require().EqualError(err, "addresses cannot be empty: unknown address")
err = types.VerifyAddressFormat(addr5)
s.Require().NoError(err)
err = types.VerifyAddressFormat(addr20)
s.Require().NoError(err)
err = types.VerifyAddressFormat(addr32)
s.Require().NoError(err)
err = types.VerifyAddressFormat(addr256)
s.Require().EqualError(err, "address max length is 255, got 256: unknown address")
}

func (s *addressTestSuite) TestCustomAddressVerifier() {
// Create a 10 byte address
addr := []byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9}
accBech := types.AccAddress(addr).String()
valBech := types.ValAddress(addr).String()
consBech := types.ConsAddress(addr).String()
// Verify that the default logic doesn't reject this 10 byte address
// The default verifier is nil, we're only checking address length is
// between 1-255 bytes.
err := types.VerifyAddressFormat(addr)
s.Require().Nil(err)
_, err = types.AccAddressFromBech32(accBech)
s.Require().Nil(err)
_, err = types.ValAddressFromBech32(valBech)
s.Require().Nil(err)
_, err = types.ConsAddressFromBech32(consBech)
s.Require().Nil(err)

// Set a custom address verifier only accepts 20 byte addresses
types.GetConfig().SetAddressVerifier(func(bz []byte) error {
n := len(bz)
if n == 20 {
return nil
}
return fmt.Errorf("incorrect address length %d", n)
})

// Verify that the custom logic rejects this 10 byte address
err = types.VerifyAddressFormat(addr)
s.Require().NotNil(err)
_, err = types.AccAddressFromBech32(accBech)
s.Require().NotNil(err)
_, err = types.ValAddressFromBech32(valBech)
s.Require().NotNil(err)
_, err = types.ConsAddressFromBech32(consBech)
s.Require().NotNil(err)

// Reinitialize the global config to default address verifier (nil)
types.GetConfig().SetAddressVerifier(nil)
}

func (s *addressTestSuite) TestBech32ifyAddressBytes() {
addr10byte := []byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9}
addr20byte := []byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19}
Expand Down
20 changes: 0 additions & 20 deletions types/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ const DefaultKeyringServiceName = "cosmos"
type Config struct {
fullFundraiserPath string
bech32AddressPrefix map[string]string
addressVerifier func([]byte) error
mtx sync.RWMutex

sealed bool
Expand Down Expand Up @@ -97,13 +96,6 @@ func (config *Config) SetBech32PrefixForConsensusNode(addressPrefix, pubKeyPrefi
config.bech32AddressPrefix["consensus_pub"] = pubKeyPrefix
}

// SetAddressVerifier builds the Config with the provided function for verifying that addresses
// have the correct format
func (config *Config) SetAddressVerifier(addressVerifier func([]byte) error) {
config.assertNotSealed()
config.addressVerifier = addressVerifier
}

// Set the FullFundraiserPath (BIP44Prefix) on the config.
//
// Deprecated: This method is supported for backward compatibility only and will be removed in a future release. Use SetPurpose and SetCoinType instead.
Expand Down Expand Up @@ -159,18 +151,6 @@ func (config *Config) GetBech32ConsensusPubPrefix() string {
return config.bech32AddressPrefix["consensus_pub"]
}

// GetAddressVerifier returns the function to verify that addresses have the correct format
func (config *Config) GetAddressVerifier() func([]byte) error {
return config.addressVerifier
}

// GetFullFundraiserPath returns the BIP44Prefix.
//
// Deprecated: This method is supported for backward compatibility only and will be removed in a future release. Use GetFullBIP44Path instead.
func (config *Config) GetFullFundraiserPath() string {
return config.fullFundraiserPath
}

func KeyringServiceName() string {
if len(version.Name) == 0 {
return DefaultKeyringServiceName
Expand Down
12 changes: 0 additions & 12 deletions types/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,6 @@ func TestConfigTestSuite(t *testing.T) {
suite.Run(t, new(configTestSuite))
}

func (s *configTestSuite) TestConfig_SetFullFundraiserPath() {
config := sdk.NewConfig()
config.SetFullFundraiserPath("test/path")
s.Require().Equal("test/path", config.GetFullFundraiserPath())

config.SetFullFundraiserPath("test/poth")
s.Require().Equal("test/poth", config.GetFullFundraiserPath())

config.Seal()
s.Require().Panics(func() { config.SetFullFundraiserPath("x/test/path") })
}

func (s *configTestSuite) TestKeyringServiceName() {
s.Require().Equal(sdk.DefaultKeyringServiceName, sdk.KeyringServiceName())
}
6 changes: 2 additions & 4 deletions x/auth/types/credentials_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,11 @@ func TestNewModuleCrendentials(t *testing.T) {

expected := sdk.MustAccAddressFromBech32("cosmos1fpn0w0yf4x300llf5r66jnfhgj4ul6cfahrvqsskwkhsw6sv84wsmz359y")

credential, err := authtypes.NewModuleCredential("group")
_, err = authtypes.NewModuleCredential("group")
require.NoError(t, err, "must be able to create a Root Module credential (see ADR-33)")
require.NoError(t, sdk.VerifyAddressFormat(credential.Address()))

credential, err = authtypes.NewModuleCredential("group", [][]byte{{0x20}, {0x0}}...)
credential, err := authtypes.NewModuleCredential("group", [][]byte{{0x20}, {0x0}}...)
require.NoError(t, err)
require.NoError(t, sdk.VerifyAddressFormat(credential.Address()))
addr, err := sdk.AccAddressFromHexUnsafe(credential.Address().String())
require.NoError(t, err)
require.Equal(t, expected.String(), addr.String())
Expand Down
Loading