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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(x/auth): removal of Address.String() #19997

Closed
wants to merge 19 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 6 additions & 1 deletion x/auth/ante/fee.go
Original file line number Diff line number Diff line change
Expand Up @@ -112,11 +112,16 @@ func (dfd DeductFeeDecorator) checkDeductFee(ctx sdk.Context, sdkTx sdk.Tx, fee
}
}

deductFeesFromAddr, err := dfd.accountKeeper.AddressCodec().BytesToString(deductFeesFrom)
if err != nil {
return err
}

events := sdk.Events{
sdk.NewEvent(
sdk.EventTypeTx,
sdk.NewAttribute(sdk.AttributeKeyFee, fee.String()),
sdk.NewAttribute(sdk.AttributeKeyFeePayer, sdk.AccAddress(deductFeesFrom).String()),
sdk.NewAttribute(sdk.AttributeKeyFeePayer, deductFeesFromAddr),
),
}
ctx.EventManager().EmitEvents(events)
Expand Down
21 changes: 17 additions & 4 deletions x/auth/ante/sigverify.go
Original file line number Diff line number Diff line change
Expand Up @@ -340,8 +340,13 @@ func (svd SigVerificationDecorator) verifySig(ctx sdk.Context, tx sdk.Tx, acc sd

anyPk, _ := codectypes.NewAnyWithValue(pubKey)

addr, err := svd.ak.AddressCodec().BytesToString(acc.GetAddress())
if err != nil {
return err
}

signerData := txsigning.SignerData{
Address: acc.GetAddress().String(),
Address: addr,
ChainID: chainID,
AccountNumber: accNum,
Sequence: acc.GetSequence(),
JulianToledano marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -355,7 +360,7 @@ func (svd SigVerificationDecorator) verifySig(ctx sdk.Context, tx sdk.Tx, acc sd
return fmt.Errorf("expected tx to implement V2AdaptableTx, got %T", tx)
}
txData := adaptableTx.GetSigningTxData()
err := authsigning.VerifySignature(ctx, pubKey, signerData, sig.Data, svd.signModeHandler, txData)
err = authsigning.VerifySignature(ctx, pubKey, signerData, sig.Data, svd.signModeHandler, txData)
if err != nil {
var errMsg string
if OnlyLegacyAminoSigners(sig.Data) {
Expand Down Expand Up @@ -386,7 +391,11 @@ func (svd SigVerificationDecorator) setPubKey(ctx sdk.Context, acc sdk.AccountI,
// if we're not in simulation mode, and we do not have a valid pubkey
// for this signer, then we simply error.
if svd.ak.Environment().TransactionService.ExecMode(ctx) != transaction.ExecModeSimulate {
return fmt.Errorf("the account %s is without a pubkey and did not provide a pubkey in the tx to set it", acc.GetAddress().String())
addr, err := svd.ak.AddressCodec().BytesToString(acc.GetAddress())
if err != nil {
return fmt.Errorf("could not convert address to string: %s", err.Error())
}
return fmt.Errorf("the account %s is without a pubkey and did not provide a pubkey in the tx to set it", addr)
Comment on lines +394 to +398
Copy link
Contributor

Choose a reason for hiding this comment

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

Improve error handling by separating the error message construction from the error check for clarity and maintainability.

			addr, err := svd.ak.AddressCodec().BytesToString(acc.GetAddress())
			if err != nil {
-				return fmt.Errorf("could not convert address to string: %s", err.Error())
+				errMsg := fmt.Errorf("could not convert address to string: %s", err.Error())
+				return errMsg
			}

This refactor makes the error handling more readable and maintainable by clearly separating the creation of the error message from the return statement.


Committable suggestion

鈥硷笍 IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
addr, err := svd.ak.AddressCodec().BytesToString(acc.GetAddress())
if err != nil {
return fmt.Errorf("could not convert address to string: %s", err.Error())
}
return fmt.Errorf("the account %s is without a pubkey and did not provide a pubkey in the tx to set it", addr)
addr, err := svd.ak.AddressCodec().BytesToString(acc.GetAddress())
if err != nil {
errMsg := fmt.Errorf("could not convert address to string: %s", err.Error())
return errMsg
}
return fmt.Errorf("the account %s is without a pubkey and did not provide a pubkey in the tx to set it", addr)

}
// if we're in simulation mode, then we can populate the pubkey with the
// sim one and simply return.
Expand All @@ -398,7 +407,11 @@ func (svd SigVerificationDecorator) setPubKey(ctx sdk.Context, acc sdk.AccountI,
// if the address does not match the pubkey, then we error.
// TODO: in the future the relationship between address and pubkey should be more flexible.
if !acc.GetAddress().Equals(sdk.AccAddress(txPubKey.Address().Bytes())) {
return sdkerrors.ErrInvalidPubKey.Wrapf("the account %s cannot be claimed by public key with address %x", acc.GetAddress(), txPubKey.Address())
addr, err := svd.ak.AddressCodec().BytesToString(acc.GetAddress())
if err != nil {
return fmt.Errorf("could not convert address to string: %s", err.Error())
}
return sdkerrors.ErrInvalidPubKey.Wrapf("the account %s cannot be claimed by public key with address %x", addr, txPubKey.Address())
Comment on lines +410 to +414
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure consistent error handling by using sdkerrors.ErrInvalidPubKey.Wrapf for all public key related errors.

		addr, err := svd.ak.AddressCodec().BytesToString(acc.GetAddress())
		if err != nil {
-			return fmt.Errorf("could not convert address to string: %s", err.Error())
+			return sdkerrors.ErrInvalidPubKey.Wrapf("could not convert address to string: %s", err.Error())
		}
-		return sdkerrors.ErrInvalidPubKey.Wrapf("the account %s cannot be claimed by public key with address %x", addr, txPubKey.Address())
+		return sdkerrors.ErrInvalidPubKey.Wrapf("the account %s cannot be claimed by public key with address %s", addr, txPubKey.Address().String())

This change ensures that all public key related errors are handled consistently using the sdkerrors.ErrInvalidPubKey.Wrapf method, improving the readability and maintainability of the code. Additionally, converting the address to a string for the error message enhances clarity.


Committable suggestion

鈥硷笍 IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
addr, err := svd.ak.AddressCodec().BytesToString(acc.GetAddress())
if err != nil {
return fmt.Errorf("could not convert address to string: %s", err.Error())
}
return sdkerrors.ErrInvalidPubKey.Wrapf("the account %s cannot be claimed by public key with address %x", addr, txPubKey.Address())
addr, err := svd.ak.AddressCodec().BytesToString(acc.GetAddress())
if err != nil {
return sdkerrors.ErrInvalidPubKey.Wrapf("could not convert address to string: %s", err.Error())
}
return sdkerrors.ErrInvalidPubKey.Wrapf("the account %s cannot be claimed by public key with address %s", addr, txPubKey.Address().String())

}

err := verifyIsOnCurve(txPubKey)
Expand Down
6 changes: 6 additions & 0 deletions x/auth/ante/sigverify_internal_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,14 @@ import (

"github.com/stretchr/testify/require"

coreaddress "cosmossdk.io/core/address"
"cosmossdk.io/core/appmodule"
"cosmossdk.io/core/transaction"
"cosmossdk.io/x/auth/ante"
authcodec "cosmossdk.io/x/auth/codec"
authtypes "cosmossdk.io/x/auth/types"

"github.com/cosmos/cosmos-sdk/codec/address"
"github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1"
sdk "github.com/cosmos/cosmos-sdk/types"
)
Expand All @@ -26,6 +28,10 @@ func (*mockAccount) Environment() appmodule.Environment {
}
}

func (*mockAccount) AddressCodec() coreaddress.Codec {
return address.NewBech32Codec("cosmos")
}

type mockTransactionService struct {
transaction.Service
}
Expand Down
15 changes: 11 additions & 4 deletions x/auth/ante/testutil_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,12 +79,15 @@ func SetupTestSuite(t *testing.T, isCheckTx bool) *AnteTestSuite {
"random": {"random"},
}

ac := authcodec.NewBech32Codec("cosmos")
authorityAddr, err := ac.BytesToString(types.NewModuleAddress("gov"))
require.NoError(t, err)
suite.accountKeeper = keeper.NewAccountKeeper(
runtime.NewEnvironment(runtime.NewKVStoreService(key), log.NewNopLogger()), suite.encCfg.Codec, types.ProtoBaseAccount, maccPerms, authcodec.NewBech32Codec("cosmos"),
sdk.Bech32MainPrefix, types.NewModuleAddress("gov").String(), nil,
runtime.NewEnvironment(runtime.NewKVStoreService(key), log.NewNopLogger()), suite.encCfg.Codec,
types.ProtoBaseAccount, maccPerms, ac, sdk.Bech32MainPrefix, authorityAddr, nil,
)
suite.accountKeeper.GetModuleAccount(suite.ctx, types.FeeCollectorName)
err := suite.accountKeeper.Params.Set(suite.ctx, types.DefaultParams())
err = suite.accountKeeper.Params.Set(suite.ctx, types.DefaultParams())
require.NoError(t, err)

// We're using TestMsg encoding in some tests, so register it here.
Expand Down Expand Up @@ -241,8 +244,12 @@ func (suite *AnteTestSuite) CreateTestTx(
// Second round: all signer infos are set, so each signer can sign.
sigsV2 = []signing.SignatureV2{}
for i, priv := range privs {
addr, err := suite.accountKeeper.AddressCodec().BytesToString(priv.PubKey().Address())
if err != nil {
return nil, err
}
signerData := xauthsigning.SignerData{
Address: sdk.AccAddress(priv.PubKey().Address()).String(),
Address: addr,
ChainID: chainID,
AccountNumber: accNums[i],
Sequence: accSeqs[i],
Expand Down
13 changes: 11 additions & 2 deletions x/auth/client/cli/tx_multisign.go
Original file line number Diff line number Diff line change
Expand Up @@ -136,11 +136,15 @@ func makeMultiSignCmd() func(cmd *cobra.Command, args []string) (err error) {
if err != nil {
return err
}
sigAddr, err := clientCtx.AddressCodec.BytesToString(sig.PubKey.Address())
if err != nil {
return err
}
txSignerData := txsigning.SignerData{
ChainID: txFactory.ChainID(),
AccountNumber: txFactory.AccountNumber(),
Sequence: txFactory.Sequence(),
Address: sdk.AccAddress(sig.PubKey.Address()).String(),
Address: sigAddr,
PubKey: &anypb.Any{
TypeUrl: anyPk.TypeUrl,
Value: anyPk.Value,
Expand Down Expand Up @@ -310,6 +314,11 @@ func makeBatchMultisignCmd() func(cmd *cobra.Command, args []string) error {
multisigPub := pubKey.(*kmultisig.LegacyAminoPubKey)
multisigSig := multisig.NewMultisig(len(multisigPub.PubKeys))

address, err := clientCtx.AddressCodec.BytesToString(pubKey.Address())
if err != nil {
return err
}

anyPk, err := codectypes.NewAnyWithValue(multisigPub)
if err != nil {
return err
Expand All @@ -318,7 +327,7 @@ func makeBatchMultisignCmd() func(cmd *cobra.Command, args []string) error {
ChainID: txFactory.ChainID(),
AccountNumber: txFactory.AccountNumber(),
Sequence: txFactory.Sequence(),
Address: sdk.AccAddress(pubKey.Address()).String(),
Address: address,
PubKey: &anypb.Any{
TypeUrl: anyPk.TypeUrl,
Value: anyPk.Value,
Expand Down
10 changes: 8 additions & 2 deletions x/auth/client/cli/validate_sigs.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,12 @@ func printAndValidateSigs(
success = false
}

sigStrAddr, err := clientCtx.AddressCodec.BytesToString(sigAddr)
if err != nil {
cmd.PrintErrf("failed to convert address to string: %s\n", sigAddr)
return false
}
JulianToledano marked this conversation as resolved.
Show resolved Hide resolved

// validate the actual signature over the transaction bytes since we can
// reach out to a full node to query accounts.
if !offline && success {
Expand All @@ -119,7 +125,7 @@ func printAndValidateSigs(
}

signingData := authsigning.SignerData{
Address: sigAddr.String(),
Address: sigStrAddr,
ChainID: chainID,
AccountNumber: accNum,
Sequence: accSeq,
Expand Down Expand Up @@ -155,7 +161,7 @@ func printAndValidateSigs(
}
}

cmd.Printf(" %d: %s\t\t\t[%s]%s%s\n", i, sigAddr.String(), sigSanity, multiSigHeader, multiSigMsg)
cmd.Printf(" %d: %s\t\t\t[%s]%s%s\n", i, sigStrAddr, sigSanity, multiSigHeader, multiSigMsg)
}

cmd.Println("")
Expand Down
2 changes: 1 addition & 1 deletion x/auth/client/testutil/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ func TxMultiSignExec(clientCtx client.Context, from, filename string, extraArgs

func TxSignBatchExec(clientCtx client.Context, from fmt.Stringer, filename string, extraArgs ...string) (testutil.BufferWriter, error) {
args := []string{
fmt.Sprintf("--from=%s", from.String()),
fmt.Sprintf("--from=%s", from),
filename,
}

Expand Down
41 changes: 31 additions & 10 deletions x/auth/keeper/deterministic_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,14 +71,18 @@ func (suite *DeterministicTestSuite) SetupTest() {
randomPerm: {"random"},
}

ac := authcodec.NewBech32Codec("cosmos")
authorityAddr, err := ac.BytesToString(types.NewModuleAddress("gov"))
suite.Require().NoError(err)

suite.accountKeeper = keeper.NewAccountKeeper(
env,
suite.encCfg.Codec,
types.ProtoBaseAccount,
maccPerms,
authcodec.NewBech32Codec("cosmos"),
ac,
"cosmos",
types.NewModuleAddress("gov").String(),
authorityAddr,
nil,
)

Expand Down Expand Up @@ -123,7 +127,9 @@ func (suite *DeterministicTestSuite) createAndSetAccounts(t *rapid.T, count int)
func (suite *DeterministicTestSuite) TestGRPCQueryAccount() {
rapid.Check(suite.T(), func(t *rapid.T) {
accs := suite.createAndSetAccounts(t, 1)
req := &types.QueryAccountRequest{Address: accs[0].GetAddress().String()}
accAddr, err := suite.accountKeeper.AddressCodec().BytesToString(accs[0].GetAddress())
suite.Require().NoError(err)
req := &types.QueryAccountRequest{Address: accAddr}
testdata.DeterministicIterations(suite.T(), suite.ctx, req, suite.queryClient.Account, 0, true)
})

Expand All @@ -134,7 +140,10 @@ func (suite *DeterministicTestSuite) TestGRPCQueryAccount() {
acc1 := types.NewBaseAccount(addr, &secp256k1.PubKey{Key: pub}, accNum, seq)
suite.accountKeeper.SetAccount(suite.ctx, acc1)

req := &types.QueryAccountRequest{Address: acc1.GetAddress().String()}
accAddr, err := suite.accountKeeper.AddressCodec().BytesToString(acc1.GetAddress())
suite.Require().NoError(err)

req := &types.QueryAccountRequest{Address: accAddr}

testdata.DeterministicIterations(suite.T(), suite.ctx, req, suite.queryClient.Account, 1543, false)
}
Expand Down Expand Up @@ -230,7 +239,10 @@ func (suite *DeterministicTestSuite) TestGRPCQueryAccountInfo() {
accs := suite.createAndSetAccounts(t, 1)
suite.Require().Len(accs, 1)

req := &types.QueryAccountInfoRequest{Address: accs[0].GetAddress().String()}
acc0Addr, err := suite.accountKeeper.AddressCodec().BytesToString(accs[0].GetAddress())
suite.Require().NoError(err)

req := &types.QueryAccountInfoRequest{Address: acc0Addr}
testdata.DeterministicIterations(suite.T(), suite.ctx, req, suite.queryClient.AccountInfo, 0, true)
})

Expand All @@ -239,9 +251,11 @@ func (suite *DeterministicTestSuite) TestGRPCQueryAccountInfo() {
seq := uint64(10)

acc := types.NewBaseAccount(addr, &secp256k1.PubKey{Key: pub}, accNum, seq)
accAddr, err := suite.accountKeeper.AddressCodec().BytesToString(acc.GetAddress())
suite.Require().NoError(err)

suite.accountKeeper.SetAccount(suite.ctx, acc)
req := &types.QueryAccountInfoRequest{Address: acc.GetAddress().String()}
req := &types.QueryAccountInfoRequest{Address: accAddr}
testdata.DeterministicIterations(suite.T(), suite.ctx, req, suite.queryClient.AccountInfo, 1543, false)
}

Expand Down Expand Up @@ -292,14 +306,17 @@ func (suite *DeterministicTestSuite) TestGRPCQueryModuleAccounts() {
maccPerms[maccs[i]] = mPerms
}

authorityAddr, err := ac.BytesToString(types.NewModuleAddress("gov"))
suite.Require().NoError(err)

ak := keeper.NewAccountKeeper(
suite.environment,
suite.encCfg.Codec,
types.ProtoBaseAccount,
maccPerms,
authcodec.NewBech32Codec("cosmos"),
ac,
"cosmos",
types.NewModuleAddress("gov").String(),
authorityAddr,
nil,
)
suite.setModuleAccounts(suite.ctx, ak, maccs)
Expand Down Expand Up @@ -340,14 +357,18 @@ func (suite *DeterministicTestSuite) TestGRPCQueryModuleAccountByName() {

maccPerms[mName] = mPerms

ac := authcodec.NewBech32Codec("cosmos")
authorityAddr, err := ac.BytesToString(types.NewModuleAddress("gov"))
suite.Require().NoError(err)

ak := keeper.NewAccountKeeper(
suite.environment,
suite.encCfg.Codec,
types.ProtoBaseAccount,
maccPerms,
authcodec.NewBech32Codec("cosmos"),
ac,
"cosmos",
types.NewModuleAddress("gov").String(),
authorityAddr,
nil,
)
suite.setModuleAccounts(suite.ctx, ak, []string{mName})
Expand Down
19 changes: 13 additions & 6 deletions x/auth/keeper/grpc_query_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@ func (suite *KeeperTestSuite) TestGRPCQueryAccounts() {
func (suite *KeeperTestSuite) TestGRPCQueryAccount() {
var req *types.QueryAccountRequest
_, _, addr := testdata.KeyTestPubAddr()
addrString, err := suite.accountKeeper.AddressCodec().BytesToString(addr)
suite.Require().NoError(err)

testCases := []struct {
msg string
Expand Down Expand Up @@ -110,7 +112,7 @@ func (suite *KeeperTestSuite) TestGRPCQueryAccount() {
{
"account not found",
func() {
req = &types.QueryAccountRequest{Address: addr.String()}
req = &types.QueryAccountRequest{Address: addrString}
},
false,
func(res *types.QueryAccountResponse) {},
Expand All @@ -120,7 +122,7 @@ func (suite *KeeperTestSuite) TestGRPCQueryAccount() {
func() {
suite.accountKeeper.SetAccount(suite.ctx,
suite.accountKeeper.NewAccountWithAddress(suite.ctx, addr))
req = &types.QueryAccountRequest{Address: addr.String()}
req = &types.QueryAccountRequest{Address: addrString}
},
true,
func(res *types.QueryAccountResponse) {
Expand Down Expand Up @@ -508,13 +510,16 @@ func (suite *KeeperTestSuite) TestQueryAccountInfo() {
suite.Require().NoError(acc.SetSequence(10))
suite.accountKeeper.SetAccount(suite.ctx, acc)

addrString, err := suite.accountKeeper.AddressCodec().BytesToString(addr)
suite.Require().NoError(err)

res, err := suite.queryClient.AccountInfo(context.Background(), &types.QueryAccountInfoRequest{
Address: addr.String(),
Address: addrString,
})

suite.Require().NoError(err)
suite.Require().NotNil(res.Info)
suite.Require().Equal(addr.String(), res.Info.Address)
suite.Require().Equal(addrString, res.Info.Address)
suite.Require().Equal(acc.GetAccountNumber(), res.Info.AccountNumber)
suite.Require().Equal(acc.GetSequence(), res.Info.Sequence)
suite.Require().Equal("/"+proto.MessageName(pk), res.Info.PubKey.TypeUrl)
Expand All @@ -526,13 +531,15 @@ func (suite *KeeperTestSuite) TestQueryAccountInfo() {
func (suite *KeeperTestSuite) TestQueryAccountInfoWithoutPubKey() {
acc := suite.accountKeeper.NewAccountWithAddress(suite.ctx, addr)
suite.accountKeeper.SetAccount(suite.ctx, acc)
addrString, err := suite.accountKeeper.AddressCodec().BytesToString(addr)
suite.Require().NoError(err)

res, err := suite.queryClient.AccountInfo(context.Background(), &types.QueryAccountInfoRequest{
Address: addr.String(),
Address: addrString,
})

suite.Require().NoError(err)
suite.Require().NotNil(res.Info)
suite.Require().Equal(addr.String(), res.Info.Address)
suite.Require().Equal(addrString, res.Info.Address)
suite.Require().Nil(res.Info.PubKey)
}