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: remove cyclic authz dependencies #16509

Merged
merged 10 commits into from
Jun 13, 2023
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* remove `Keeper`: `SetDelegatorWithdrawAddr`, `DeleteDelegatorWithdrawAddr`, `IterateDelegatorWithdrawAddrs`.
* (x/distribution) [#16459](https://github.com/cosmos/cosmos-sdk/pull/16459) use collections for `ValidatorCurrentRewards` state management:
* remove `Keeper`: `IterateValidatorCurrentRewards`, `GetValidatorCurrentRewards`, `SetValidatorCurrentRewards`, `DeleteValidatorCurrentRewards`
* (x/authz) [#16509](https://github.com/cosmos/cosmos-sdk/pull/16509) `AcceptResponse` has been moved to sdk/types/authz and the `Updated` field is now of the type `sdk.Msg` instead of `authz.Authorization`.

## [v0.50.0-alpha.0](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.50.0-alpha.0) - 2023-06-07

Expand Down
18 changes: 18 additions & 0 deletions types/authz/authorizations.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package authz

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

// AcceptResponse instruments the controller of an authz message if the request is accepted
// and if it should be updated or deleted.
type AcceptResponse struct {
// If Accept=true, the controller can accept and authorization and handle the update.
Accept bool
// If Delete=true, the controller must delete the authorization object and release
// storage resources.
Delete bool
// Controller, who is calling Authorization.Accept must check if `Updated != nil`. If yes,
// it must use the updated version and handle the update on the storage level.
Updated sdk.Msg
}
16 changes: 2 additions & 14 deletions x/authz/authorizations.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"github.com/cosmos/gogoproto/proto"

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

// Authorization represents the interface of various Authorization types implemented
Expand All @@ -19,22 +20,9 @@ type Authorization interface {

// Accept determines whether this grant permits the provided sdk.Msg to be performed,
// and if so provides an upgraded authorization instance.
Accept(ctx context.Context, msg sdk.Msg) (AcceptResponse, error)
Accept(ctx context.Context, msg sdk.Msg) (authz.AcceptResponse, error)

// ValidateBasic does a simple validation check that
// doesn't require access to any other information.
ValidateBasic() error
}

// AcceptResponse instruments the controller of an authz message if the request is accepted
// and if it should be updated or deleted.
type AcceptResponse struct {
// If Accept=true, the controller can accept and authorization and handle the update.
Accept bool
// If Delete=true, the controller must delete the authorization object and release
// storage resources.
Delete bool
// Controller, who is calling Authorization.Accept must check if `Updated != nil`. If yes,
// it must use the updated version and handle the update on the storage level.
Updated Authorization
}
9 changes: 8 additions & 1 deletion x/authz/codec.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import (
types "github.com/cosmos/cosmos-sdk/codec/types"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/msgservice"
bank "github.com/cosmos/cosmos-sdk/x/bank/types"
staking "github.com/cosmos/cosmos-sdk/x/staking/types"
)

// RegisterLegacyAminoCodec registers the necessary x/authz interfaces and concrete types
Expand All @@ -27,11 +29,16 @@ func RegisterInterfaces(registry types.InterfaceRegistry) {
&MsgExec{},
)

// since bank.SendAuthorization and staking.StakeAuthorization both implement Authorization
// and authz depends on x/bank and x/staking in other places, these registrations are placed here
// to prevent a cyclic dependency.
// see: https://github.com/cosmos/cosmos-sdk/pull/16509
registry.RegisterInterface(
"cosmos.authz.v1beta1.Authorization",
(*Authorization)(nil),
&GenericAuthorization{},
&bank.SendAuthorization{},
&staking.StakeAuthorization{},
)

msgservice.RegisterMsgServiceDesc(registry, MsgServiceDesc())
}
9 changes: 4 additions & 5 deletions x/authz/generic_authorization.go
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
package authz

import (
context "context"
"context"

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

var _ Authorization = &GenericAuthorization{}

// NewGenericAuthorization creates a new GenericAuthorization object.
func NewGenericAuthorization(msgTypeURL string) *GenericAuthorization {
return &GenericAuthorization{
Expand All @@ -21,8 +20,8 @@ func (a GenericAuthorization) MsgTypeURL() string {
}

// Accept implements Authorization.Accept.
func (a GenericAuthorization) Accept(ctx context.Context, msg sdk.Msg) (AcceptResponse, error) {
return AcceptResponse{Accept: true}, nil
func (a GenericAuthorization) Accept(ctx context.Context, msg sdk.Msg) (authz.AcceptResponse, error) {
return authz.AcceptResponse{Accept: true}, nil
}

// ValidateBasic implements Authorization.ValidateBasic.
Expand Down
6 changes: 5 additions & 1 deletion x/authz/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,11 @@ func (k Keeper) DispatchActions(ctx context.Context, grantee sdk.AccAddress, msg
if resp.Delete {
err = k.DeleteGrant(ctx, grantee, granter, sdk.MsgTypeURL(msg))
} else if resp.Updated != nil {
err = k.update(ctx, grantee, granter, resp.Updated)
updated, ok := resp.Updated.(authz.Authorization)
if !ok {
return nil, fmt.Errorf("expected authz.Authorization but got %T", resp.Updated)
}
err = k.update(ctx, grantee, granter, updated)
}
if err != nil {
return nil, err
Expand Down
5 changes: 0 additions & 5 deletions x/bank/types/codec.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
"github.com/cosmos/cosmos-sdk/codec/types"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/msgservice"
"github.com/cosmos/cosmos-sdk/x/authz"
)

// RegisterLegacyAminoCodec registers the necessary x/bank interfaces and concrete types
Expand All @@ -27,10 +26,6 @@ func RegisterInterfaces(registry types.InterfaceRegistry) {
&MsgMultiSend{},
&MsgUpdateParams{},
)
registry.RegisterImplementations(
(*authz.Authorization)(nil),
&SendAuthorization{},
)

msgservice.RegisterMsgServiceDesc(registry, &_Msg_serviceDesc)
}
4 changes: 1 addition & 3 deletions x/bank/types/send_authorization.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,15 @@ import (
context "context"

sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/authz"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
"github.com/cosmos/cosmos-sdk/x/authz"
)

// TODO: Revisit this once we have proper gas fee framework.
// Ref: https://github.com/cosmos/cosmos-sdk/issues/9054
// Ref: https://github.com/cosmos/cosmos-sdk/discussions/9072
const gasCostPerIteration = uint64(10)

var _ authz.Authorization = &SendAuthorization{}

// NewSendAuthorization creates a new SendAuthorization object.
func NewSendAuthorization(spendLimit sdk.Coins, allowed []sdk.AccAddress) *SendAuthorization {
return &SendAuthorization{
Expand Down
2 changes: 1 addition & 1 deletion x/bank/types/send_authorization_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ func TestSendAuthorization(t *testing.T) {
require.Equal(t, sendAuth.String(), resp.Updated.String())

t.Log("expect updated authorization nil after spending remaining amount")
resp, err = resp.Updated.Accept(ctx, send)
resp, err = resp.Updated.(*types.SendAuthorization).Accept(ctx, send)
require.NoError(t, err)
require.True(t, resp.Delete)
require.Nil(t, resp.Updated)
Expand Down
15 changes: 8 additions & 7 deletions x/staking/types/authz.go
Original file line number Diff line number Diff line change
@@ -1,21 +1,20 @@
package types

import (
context "context"
"context"
"fmt"

errorsmod "cosmossdk.io/errors"

sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/authz"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
"github.com/cosmos/cosmos-sdk/x/authz"
)

// TODO: Revisit this once we have propoer gas fee framework.
// Tracking issues https://github.com/cosmos/cosmos-sdk/issues/9054, https://github.com/cosmos/cosmos-sdk/discussions/9072
const gasCostPerIteration = uint64(10)

var _ authz.Authorization = &StakeAuthorization{}
Copy link
Member

Choose a reason for hiding this comment

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

This is quite handy, if we go this way, cannot we move the interface in types/authz?
Codec (x/bank/types/codec.go) still requires the interface registration anyway, so it does not really remove the cyclic dep as of now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Codec (x/bank/types/codec.go) still requires the interface registration anyway, so it does not really remove the cyclic dep as of now.

Oops, I meant to move those registrations to authz. Will do that now.

This is quite handy, if we go this way, cannot we move the interface in types/authz?

This is handy while writing, but obviates one of the best features of how interfaces are implemented in go's type system. A type can implement an interface's behavior without forming an explicit relationship on the package in which it is specified.


// NewStakeAuthorization creates a new StakeAuthorization object.
func NewStakeAuthorization(allowed, denied []sdk.ValAddress, authzType AuthorizationType, amount *sdk.Coin) (*StakeAuthorization, error) {
allowedValidators, deniedValidators, err := validateAllowAndDenyValidators(allowed, denied)
Expand Down Expand Up @@ -62,11 +61,12 @@ func (a StakeAuthorization) MsgTypeURL() string {
// is unspecified.
func (a StakeAuthorization) ValidateBasic() error {
if a.MaxTokens != nil && a.MaxTokens.IsNegative() {
return errorsmod.Wrapf(authz.ErrNegativeMaxTokens, "negative coin amount: %v", a.MaxTokens)
return errorsmod.Wrapf(fmt.Errorf("max tokens should be positive"),
"negative coin amount: %v", a.MaxTokens)
}

if a.AuthorizationType == AuthorizationType_AUTHORIZATION_TYPE_UNSPECIFIED {
return authz.ErrUnknownAuthorizationType
return fmt.Errorf("unknown authorization type")
}

return nil
Expand Down Expand Up @@ -190,6 +190,7 @@ func normalizeAuthzType(authzType AuthorizationType) (string, error) {
case AuthorizationType_AUTHORIZATION_TYPE_CANCEL_UNBONDING_DELEGATION:
return sdk.MsgTypeURL(&MsgCancelUnbondingDelegation{}), nil
default:
return "", errorsmod.Wrapf(authz.ErrUnknownAuthorizationType, "cannot normalize authz type with %T", authzType)
return "", errorsmod.Wrapf(fmt.Errorf("unknown authorization type"),
"cannot normalize authz type with %T", authzType)
}
}
5 changes: 0 additions & 5 deletions x/staking/types/codec.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
"github.com/cosmos/cosmos-sdk/codec/types"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/msgservice"
"github.com/cosmos/cosmos-sdk/x/authz"
)

// RegisterLegacyAminoCodec registers the necessary x/staking interfaces and concrete types
Expand Down Expand Up @@ -38,10 +37,6 @@ func RegisterInterfaces(registry types.InterfaceRegistry) {
&MsgCancelUnbondingDelegation{},
&MsgUpdateParams{},
)
registry.RegisterImplementations(
(*authz.Authorization)(nil),
&StakeAuthorization{},
)

msgservice.RegisterMsgServiceDesc(registry, &_Msg_serviceDesc)
}