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

refactor(types)!: removed txEncoder from global config #18695

Merged
merged 5 commits into from
Dec 15, 2023
+2 −35
Merged

refactor(types)!: removed txEncoder from global config #18695

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
292798e
removed global txEncoder
bizk Dec 11, 2023
f10a2ec
updated changelog
bizk Dec 11, 2023
bcbcb98
removed unused types and fix linted
bizk Dec 11, 2023
a1420c4
Merge branch 'main' into chore/globalCfgTxEncoder
bizk Dec 15, 2023
7149c27
updated changelog
bizk Dec 15, 2023
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 @@ -76,6 +76,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (crypto | x/auth) [#14372](https://github.com/cosmos/cosmos-sdk/pull/18194) Key checks on signatures antehandle.
* (staking) [#18506](https://github.com/cosmos/cosmos-sdk/pull/18506) Detect the length of the ed25519 pubkey in CreateValidator to prevent panic.
* (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.
Copy link
Member

Choose a reason for hiding this comment

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

Can we always precise the alternative when we remove something from the config?

Maybe it would be nice to add an UPGRADING.md section on the global config removals, so there will be one place to look at it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one has not been used anywhere, I think it is deprecated, thats why I didn't add an alternative, TxEncoder is used on other parts but not the one dfined as a global config. I dont see any use cases where someone would want some sort of global txEncoder

Copy link
Member

Choose a reason for hiding this comment

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

A quick search here: https://sourcegraph.com/search?q=context%3Aglobal+.GetTxEncoder%28%29&patternType=standard&sm=1&groupBy=repo shows indeed that it is only used in v0.45 chains / fork. Then I guess it is fine indeed.

bizk marked this conversation as resolved.
Show resolved Hide resolved

### Bug Fixes

Copy link
Contributor

Choose a reason for hiding this comment

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

Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [15-15]

The changelog mentions a security vulnerability identified in the x/bank module in the v0.41.x series, which is critical and should be highlighted for users running non-Cosmos Hub chains.

Expand Down
20 changes: 0 additions & 20 deletions types/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,9 @@ const DefaultKeyringServiceName = "cosmos"
type Config struct {
fullFundraiserPath string
bech32AddressPrefix map[string]string
txEncoder TxEncoder
addressVerifier func([]byte) error
mtx sync.RWMutex

// SLIP-44 related
purpose uint32
coinType uint32

sealed bool
sealedch chan struct{}
}
Expand All @@ -46,10 +41,6 @@ func NewConfig() *Config {
"consensus_pub": Bech32PrefixConsPub,
},
fullFundraiserPath: FullFundraiserPath,

purpose: Purpose,
coinType: CoinType,
txEncoder: nil,
}
}

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

// SetTxEncoder builds the Config with TxEncoder used to marshal StdTx to bytes
func (config *Config) SetTxEncoder(encoder TxEncoder) {
config.assertNotSealed()
config.txEncoder = encoder
}

// SetAddressVerifier builds the Config with the provided function for verifying that addresses
// have the correct format
func (config *Config) SetAddressVerifier(addressVerifier func([]byte) error) {
Expand Down Expand Up @@ -174,11 +159,6 @@ func (config *Config) GetBech32ConsensusPubPrefix() string {
return config.bech32AddressPrefix["consensus_pub"]
}

// GetTxEncoder return function to encode transactions
func (config *Config) GetTxEncoder() TxEncoder {
return config.txEncoder
}

// GetAddressVerifier returns the function to verify that addresses have the correct format
func (config *Config) GetAddressVerifier() func([]byte) error {
return config.addressVerifier
Expand Down
14 changes: 0 additions & 14 deletions types/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package types_test

import (
"errors"
"testing"

"github.com/stretchr/testify/suite"
Expand All @@ -17,19 +16,6 @@ func TestConfigTestSuite(t *testing.T) {
suite.Run(t, new(configTestSuite))
}

func (s *configTestSuite) TestConfig_SetTxEncoder() {
mockErr := errors.New("test")
config := sdk.NewConfig()
s.Require().Nil(config.GetTxEncoder())
encFunc := sdk.TxEncoder(func(tx sdk.Tx) ([]byte, error) { return nil, nil })
config.SetTxEncoder(encFunc)
_, err := config.GetTxEncoder()(sdk.Tx(nil))
s.Require().Error(mockErr, err)

config.Seal()
s.Require().Panics(func() { config.SetTxEncoder(encFunc) })
}

func (s *configTestSuite) TestConfig_SetFullFundraiserPath() {
config := sdk.NewConfig()
config.SetFullFundraiserPath("test/path")
Expand Down
Loading