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

Jump to bottom

chore(x/authz)!: Remove account keeper dependency #21632

Merged
merged 9 commits into from
Oct 1, 2024
Merged

chore(x/authz)!: Remove account keeper dependency #21632

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
fe95ddf
re-do
facundomedica Sep 26, 2024
9e84f98
lint
facundomedica Sep 27, 2024
1b037bf
lint
facundomedica Sep 27, 2024
3f76520
mocks
facundomedica Sep 28, 2024
1f7069a
remove mock
facundomedica Sep 28, 2024
7ab64a6
Merge branch 'main' into facu/try-no-auth
facundomedica Sep 30, 2024
186158a
cl++
facundomedica Sep 30, 2024
1b8d338
Merge branch 'main' into facu/try-no-auth
facundomedica Sep 30, 2024
4b7c3cf
Merge branch 'facu/try-no-auth' of https://github.com/cosmos/cosmos-s…
facundomedica Sep 30, 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
2 changes: 1 addition & 1 deletion simapp/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,7 +398,7 @@ func NewSimApp(
app.CircuitKeeper = circuitkeeper.NewKeeper(runtime.NewEnvironment(runtime.NewKVStoreService(keys[circuittypes.StoreKey]), logger.With(log.ModuleKey, "x/circuit")), appCodec, govModuleAddr, app.AuthKeeper.AddressCodec())
app.BaseApp.SetCircuitBreaker(&app.CircuitKeeper)

app.AuthzKeeper = authzkeeper.NewKeeper(runtime.NewEnvironment(runtime.NewKVStoreService(keys[authzkeeper.StoreKey]), logger.With(log.ModuleKey, "x/authz"), runtime.EnvWithMsgRouterService(app.MsgServiceRouter()), runtime.EnvWithQueryRouterService(app.GRPCQueryRouter())), appCodec, app.AuthKeeper)
app.AuthzKeeper = authzkeeper.NewKeeper(runtime.NewEnvironment(runtime.NewKVStoreService(keys[authzkeeper.StoreKey]), logger.With(log.ModuleKey, "x/authz"), runtime.EnvWithMsgRouterService(app.MsgServiceRouter()), runtime.EnvWithQueryRouterService(app.GRPCQueryRouter())), appCodec, signingCtx.AddressCodec())

groupConfig := group.DefaultConfig()
/*
Expand Down
1 change: 1 addition & 0 deletions x/authz/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

### API Breaking Changes

* [#21632](https://github.com/cosmos/cosmos-sdk/pull/21632) `NewKeeper` now takes `address.Codec` instead of `authKeeper`.
* [#21044](https://github.com/cosmos/cosmos-sdk/pull/21044) `k.DispatchActions` returns a slice of byte slices of proto marshaled anys instead of a slice of byte slices of `sdk.Result.Data`.
* [#20502](https://github.com/cosmos/cosmos-sdk/pull/20502) `Accept` on the `Authorization` interface now expects the authz environment in the `context.Context`. This is already done when `Accept` is called by `k.DispatchActions`, but should be done manually if `Accept` is called directly.
* [#19783](https://github.com/cosmos/cosmos-sdk/pull/19783) Removes the use of Accounts String() method
Expand Down
9 changes: 0 additions & 9 deletions x/authz/expected_keepers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,9 @@ package authz
import (
"context"

"cosmossdk.io/core/address"

sdk "github.com/cosmos/cosmos-sdk/types"
)

// AccountKeeper defines the expected account keeper (noalias)
type AccountKeeper interface {
AddressCodec() address.Codec
GetAccount(ctx context.Context, addr sdk.AccAddress) sdk.AccountI
NewAccountWithAddress(ctx context.Context, addr sdk.AccAddress) sdk.AccountI
}

// BankKeeper defines the expected interface needed to retrieve account balances.
type BankKeeper interface {
SpendableCoins(ctx context.Context, addr sdk.AccAddress) sdk.Coins
Expand Down
8 changes: 4 additions & 4 deletions x/authz/keeper/genesis.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,11 @@ func (k Keeper) InitGenesis(ctx context.Context, data *authz.GenesisState) error
continue
}

grantee, err := k.authKeeper.AddressCodec().StringToBytes(entry.Grantee)
grantee, err := k.addrCdc.StringToBytes(entry.Grantee)
if err != nil {
return err
}
granter, err := k.authKeeper.AddressCodec().StringToBytes(entry.Granter)
granter, err := k.addrCdc.StringToBytes(entry.Granter)
if err != nil {
return err
}
Expand All @@ -44,11 +44,11 @@ func (k Keeper) InitGenesis(ctx context.Context, data *authz.GenesisState) error
func (k Keeper) ExportGenesis(ctx context.Context) (*authz.GenesisState, error) {
var entries []authz.GrantAuthorization
err := k.IterateGrants(ctx, func(granter, grantee sdk.AccAddress, grant authz.Grant) (bool, error) {
granterAddr, err := k.authKeeper.AddressCodec().BytesToString(granter)
granterAddr, err := k.addrCdc.BytesToString(granter)
if err != nil {
return false, err
}
granteeAddr, err := k.authKeeper.AddressCodec().BytesToString(grantee)
granteeAddr, err := k.addrCdc.BytesToString(grantee)
if err != nil {
return false, err
}
Expand Down
21 changes: 7 additions & 14 deletions x/authz/keeper/genesis_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"testing"
"time"

"github.com/golang/mock/gomock"
"github.com/stretchr/testify/suite"

"cosmossdk.io/core/header"
Expand All @@ -14,11 +13,10 @@ import (
storetypes "cosmossdk.io/store/types"
"cosmossdk.io/x/authz/keeper"
authzmodule "cosmossdk.io/x/authz/module"
authztestutil "cosmossdk.io/x/authz/testutil"
bank "cosmossdk.io/x/bank/types"

"github.com/cosmos/cosmos-sdk/baseapp"
"github.com/cosmos/cosmos-sdk/codec/address"
addresscodec "github.com/cosmos/cosmos-sdk/codec/address"
codectestutil "github.com/cosmos/cosmos-sdk/codec/testutil"
"github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1"
"github.com/cosmos/cosmos-sdk/runtime"
Expand All @@ -37,11 +35,10 @@ var (
type GenesisTestSuite struct {
suite.Suite

ctx sdk.Context
keeper keeper.Keeper
baseApp *baseapp.BaseApp
accountKeeper *authztestutil.MockAccountKeeper
encCfg moduletestutil.TestEncodingConfig
ctx sdk.Context
keeper keeper.Keeper
baseApp *baseapp.BaseApp
encCfg moduletestutil.TestEncodingConfig
}

func (suite *GenesisTestSuite) SetupTest() {
Expand All @@ -52,11 +49,6 @@ func (suite *GenesisTestSuite) SetupTest() {

suite.encCfg = moduletestutil.MakeTestEncodingConfig(codectestutil.CodecOptions{}, authzmodule.AppModule{})

// gomock initializations
ctrl := gomock.NewController(suite.T())
suite.accountKeeper = authztestutil.NewMockAccountKeeper(ctrl)
suite.accountKeeper.EXPECT().AddressCodec().Return(address.NewBech32Codec("cosmos")).AnyTimes()

suite.baseApp = baseapp.NewBaseApp(
"authz",
log.NewNopLogger(),
Expand All @@ -71,7 +63,8 @@ func (suite *GenesisTestSuite) SetupTest() {
msr.SetInterfaceRegistry(suite.encCfg.InterfaceRegistry)
env := runtime.NewEnvironment(storeService, coretesting.NewNopLogger(), runtime.EnvWithMsgRouterService(msr))

suite.keeper = keeper.NewKeeper(env, suite.encCfg.Codec, suite.accountKeeper)
addrCdc := addresscodec.NewBech32Codec("cosmos")
suite.keeper = keeper.NewKeeper(env, suite.encCfg.Codec, addrCdc)
}

func (suite *GenesisTestSuite) TestImportExportGenesis() {
Expand Down
12 changes: 6 additions & 6 deletions x/authz/keeper/grpc_query.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,12 @@ func (k Keeper) Grants(ctx context.Context, req *authz.QueryGrantsRequest) (*aut
return nil, status.Errorf(codes.InvalidArgument, "empty request")
}

granter, err := k.authKeeper.AddressCodec().StringToBytes(req.Granter)
granter, err := k.addrCdc.StringToBytes(req.Granter)
if err != nil {
return nil, err
}

grantee, err := k.authKeeper.AddressCodec().StringToBytes(req.Grantee)
grantee, err := k.addrCdc.StringToBytes(req.Grantee)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -95,7 +95,7 @@ func (k Keeper) GranterGrants(ctx context.Context, req *authz.QueryGranterGrants
return nil, status.Errorf(codes.InvalidArgument, "empty request")
}

granter, err := k.authKeeper.AddressCodec().StringToBytes(req.Granter)
granter, err := k.addrCdc.StringToBytes(req.Granter)
if err != nil {
return nil, err
}
Expand All @@ -116,7 +116,7 @@ func (k Keeper) GranterGrants(ctx context.Context, req *authz.QueryGranterGrants

grantee := firstAddressFromGrantStoreKey(key)

granteeAddr, err := k.authKeeper.AddressCodec().BytesToString(grantee)
granteeAddr, err := k.addrCdc.BytesToString(grantee)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -146,7 +146,7 @@ func (k Keeper) GranteeGrants(ctx context.Context, req *authz.QueryGranteeGrants
return nil, status.Errorf(codes.InvalidArgument, "empty request")
}

grantee, err := k.authKeeper.AddressCodec().StringToBytes(req.Grantee)
grantee, err := k.addrCdc.StringToBytes(req.Grantee)
if err != nil {
return nil, err
}
Expand All @@ -169,7 +169,7 @@ func (k Keeper) GranteeGrants(ctx context.Context, req *authz.QueryGranteeGrants
return nil, status.Error(codes.Internal, err.Error())
}

granterAddr, err := k.authKeeper.AddressCodec().BytesToString(granter)
granterAddr, err := k.addrCdc.BytesToString(granter)
if err != nil {
return nil, err
}
Expand Down
12 changes: 6 additions & 6 deletions x/authz/keeper/grpc_query_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@ func (suite *TestSuite) TestGRPCQueryAuthorization() {
expAuthorization authz.Authorization
)

addr0, err := suite.accountKeeper.AddressCodec().BytesToString(addrs[0])
addr0, err := suite.addrCdc.BytesToString(addrs[0])
suite.Require().NoError(err)
addr1, err := suite.accountKeeper.AddressCodec().BytesToString(addrs[1])
addr1, err := suite.addrCdc.BytesToString(addrs[1])
suite.Require().NoError(err)

testCases := []struct {
Expand Down Expand Up @@ -137,7 +137,7 @@ func (suite *TestSuite) TestGRPCQueryGranterGrants() {
require := suite.Require()
queryClient, addrs := suite.queryClient, suite.addrs

addr0, err := suite.accountKeeper.AddressCodec().BytesToString(addrs[0])
addr0, err := suite.addrCdc.BytesToString(addrs[0])
suite.Require().NoError(err)

testCases := []struct {
Expand Down Expand Up @@ -209,9 +209,9 @@ func (suite *TestSuite) TestGRPCQueryGranteeGrants() {
require := suite.Require()
queryClient, addrs := suite.queryClient, suite.addrs

addr0, err := suite.accountKeeper.AddressCodec().BytesToString(addrs[0])
addr0, err := suite.addrCdc.BytesToString(addrs[0])
suite.Require().NoError(err)
addr2, err := suite.accountKeeper.AddressCodec().BytesToString(addrs[2])
addr2, err := suite.addrCdc.BytesToString(addrs[2])
suite.Require().NoError(err)

testCases := []struct {
Expand Down Expand Up @@ -299,7 +299,7 @@ func (suite *TestSuite) createSendAuthorization(grantee, granter sdk.AccAddress)
func (suite *TestSuite) createSendAuthorizationWithAllowList(grantee, granter sdk.AccAddress) authz.Authorization {
exp := suite.ctx.HeaderInfo().Time.Add(time.Hour)
newCoins := sdk.NewCoins(sdk.NewInt64Coin("steak", 100))
addr, err := suite.accountKeeper.AddressCodec().BytesToString(suite.addrs[5])
addr, err := suite.addrCdc.BytesToString(suite.addrs[5])
suite.Require().NoError(err)
authorization := &banktypes.SendAuthorization{SpendLimit: newCoins, AllowList: []string{addr}}
err = suite.authzKeeper.SaveGrant(suite.ctx, grantee, granter, authorization, &exp)
Expand Down
23 changes: 12 additions & 11 deletions x/authz/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
gogoproto "github.com/cosmos/gogoproto/proto"
gogoprotoany "github.com/cosmos/gogoproto/types/any"

"cosmossdk.io/core/address"
"cosmossdk.io/core/appmodule"
corecontext "cosmossdk.io/core/context"
errorsmod "cosmossdk.io/errors"
Expand All @@ -30,16 +31,16 @@ const gasCostPerIteration = uint64(20)
type Keeper struct {
appmodule.Environment

cdc codec.Codec
authKeeper authz.AccountKeeper
cdc codec.Codec
addrCdc address.Codec
}

// NewKeeper constructs a message authorization Keeper
func NewKeeper(env appmodule.Environment, cdc codec.Codec, ak authz.AccountKeeper) Keeper {
func NewKeeper(env appmodule.Environment, cdc codec.Codec, addrCdc address.Codec) Keeper {
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Multiple NewKeeper invocations have not been updated to include addrCdc address.Codec as the third parameter:

  • Instances in various test files and modules (e.g., orm/model/ormdb/module_test.go, x/upgrade/keeper/keeper_test.go, x/authz/keeper/genesis_test.go, etc.) are missing the updated parameter.
  • This inconsistency may lead to potential issues in the application's authorization mechanisms.

Recommended Actions:

  • Update all identified NewKeeper calls to include addrCdc address.Codec as the third argument.
  • Review and test the changes to ensure compatibility and correct functionality across the codebase.
🔗 Analysis chain

Verify all NewKeeper invocations are updated

Ensure that all calls to NewKeeper in the codebase have been updated to match the new signature accepting addrCdc address.Codec as the third parameter.

Run the following script to find all invocations of NewKeeper:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for all invocations of 'NewKeeper' in Go files.

# Command: Find all instances of 'NewKeeper(' in Go files.
rg --type go 'NewKeeper\('

# Expected result: All calls should use 'addrCdc address.Codec' as the third argument.

Length of output: 19562

return Keeper{
Environment: env,
cdc: cdc,
authKeeper: ak,
addrCdc: addrCdc,
}
}

Expand Down Expand Up @@ -206,11 +207,11 @@ func (k Keeper) SaveGrant(ctx context.Context, grantee, granter sdk.AccAddress,
return err
}

granterAddr, err := k.authKeeper.AddressCodec().BytesToString(granter)
granterAddr, err := k.addrCdc.BytesToString(granter)
if err != nil {
return err
Comment on lines +210 to 212
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Refactor repetitive address conversions into a helper method

The address conversion using k.addrCdc.BytesToString is repeated multiple times. To enhance maintainability and reduce duplication, consider creating a helper method for this operation.

Define the following helper method in the Keeper struct (outside the selected line ranges):

func (k Keeper) addressToString(addr sdk.AccAddress) (string, error) {
	return k.addrCdc.BytesToString(addr)
}

Then, replace the repeated code with calls to this helper method. For example, at line 210:

-	granterAddr, err := k.addrCdc.BytesToString(granter)
+	granterAddr, err := k.addressToString(granter)
	if err != nil {
		return err
	}

Apply this change to all similar occurrences where address conversion is performed.

Also applies to: 214-216, 233-235, 238-240, 259-261, 263-265, 295-297

}
granteeAddr, err := k.authKeeper.AddressCodec().BytesToString(grantee)
granteeAddr, err := k.addrCdc.BytesToString(grantee)
if err != nil {
return err
}
Expand All @@ -229,12 +230,12 @@ func (k Keeper) DeleteGrant(ctx context.Context, grantee, granter sdk.AccAddress
skey := grantStoreKey(grantee, granter, msgType)
grant, found := k.getGrant(ctx, skey)
if !found {
granterAddr, err := k.authKeeper.AddressCodec().BytesToString(granter)
granterAddr, err := k.addrCdc.BytesToString(granter)
if err != nil {
return errorsmod.Wrapf(authz.ErrNoAuthorizationFound,
"could not convert granter address to string")
}
granteeAddr, err := k.authKeeper.AddressCodec().BytesToString(grantee)
granteeAddr, err := k.addrCdc.BytesToString(grantee)
if err != nil {
return errorsmod.Wrapf(authz.ErrNoAuthorizationFound,
Comment on lines +233 to 240
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

Return appropriate error when address conversion fails

Wrapping the error with ErrNoAuthorizationFound when address conversion fails might obscure the actual issue. It's better to return the underlying error to provide clearer information for debugging.

Apply this diff to return the actual error:

	granterAddr, err := k.addrCdc.BytesToString(granter)
	if err != nil {
-		return errorsmod.Wrapf(authz.ErrNoAuthorizationFound,
-			"could not convert granter address to string")
+		return err
	}

	granteeAddr, err := k.addrCdc.BytesToString(grantee)
	if err != nil {
-		return errorsmod.Wrapf(authz.ErrNoAuthorizationFound,
-			"could not convert grantee address to string")
+		return err
	}
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
granterAddr, err := k.addrCdc.BytesToString(granter)
if err != nil {
return errorsmod.Wrapf(authz.ErrNoAuthorizationFound,
"could not convert granter address to string")
}
granteeAddr, err := k.authKeeper.AddressCodec().BytesToString(grantee)
granteeAddr, err := k.addrCdc.BytesToString(grantee)
if err != nil {
return errorsmod.Wrapf(authz.ErrNoAuthorizationFound,
granterAddr, err := k.addrCdc.BytesToString(granter)
if err != nil {
return err
}
granteeAddr, err := k.addrCdc.BytesToString(grantee)
if err != nil {
return err
}

"could not convert grantee address to string")
Expand All @@ -255,11 +256,11 @@ func (k Keeper) DeleteGrant(ctx context.Context, grantee, granter sdk.AccAddress
return err
}

granterAddr, err := k.authKeeper.AddressCodec().BytesToString(granter)
granterAddr, err := k.addrCdc.BytesToString(granter)
if err != nil {
return err
}
granteeAddr, err := k.authKeeper.AddressCodec().BytesToString(grantee)
granteeAddr, err := k.addrCdc.BytesToString(grantee)
if err != nil {
return err
}
Expand Down Expand Up @@ -291,7 +292,7 @@ func (k Keeper) DeleteAllGrants(ctx context.Context, granter sdk.AccAddress) err
}
}

grantAddr, err := k.authKeeper.AddressCodec().BytesToString(granter)
grantAddr, err := k.addrCdc.BytesToString(granter)
if err != nil {
return err
}
Expand Down
Loading
Loading