feat: refactor whitelisted_addresses #392

trinitys7 · 2024-04-25T06:34:42Z

Closes #390

…cers

testutil/app/app.go

x/denommetadata/module.go

x/sequencers/client/cli/tx.go

danwt

There might be a few unused funcs, are you sure the wiring is complete?
Could we just have 1 proposal type called UpdatePerms, which has a grant/revoke bool or enum

danwt · 2024-04-25T09:26:50Z

proto/sequencers/genesis.proto

is sequencer_operator_address a better name?

This one is the original name in sequencer module, I didn't change anything about it. So if it's required, I think we will need another chores issue for that.

danwt · 2024-04-25T09:35:11Z

x/hub-genesis/keeper/msg_server.go

+	if !m.sequencerKeeper.HasPermission(ctx, accAddr, types.ModuleName) {
 		return nil, sdkerrors.ErrUnauthorized
 	}


unauthorized or noPermission?

the hub-genesis's orginally use Unauthoried from cosmos-sdk, while the denommetadata define an error called NoPermission

x/hub-genesis/keeper/msg_server_test.go

x/sequencers/client/cli/tx.go

x/sequencers/types/keys.go

danwt · 2024-04-25T09:52:28Z

x/sequencers/types/permission.go

+	if len(p.Permissions) == 0 {
+		return errors.New("permissions field cannot be empty")
+	}


extra branch for no reason I think

Why do you think it is not needed? In this case, we can get rid of the empty proposal that happen in our chain

it's just an extra branch for no reason
let the empty proposal happen

x/sequencers/types/permission.go

x/sequencers/types/proposal_permission.go

proto/hub-genesis/genesis.proto

lacsomot · 2024-04-25T09:56:44Z

x/denommetadata/keeper/msg_server.go

@@ -33,7 +33,12 @@ func (k msgServer) CreateDenomMetadata(
 		return nil, err
 	}

-	if !k.IsAddressPermissioned(ctx, msg.SenderAddress) {


should refactor IsAddressPermissioned and reuse it

also make it private

x/denommetadata/module.go

x/sequencers/types/permission.go

x/sequencers/types/genesis.go

trinitys7 · 2024-04-25T18:08:14Z

@danwt @anhductn2001 @lacsomot guys, thanks for all the comments you leave. I think I solves most of them. Please help me check again one more time before we can merge them into rdk

zale144 · 2024-04-25T19:23:54Z

x/denommetadata/module.go

@@ -56,7 +56,7 @@ func (AppModuleBasic) ValidateGenesis(cdc codec.JSONCodec, config client.TxEncod
 	if err := cdc.UnmarshalJSON(bz, &genState); err != nil {


why bother unmashalling genState if it's not used?

zale144 · 2024-04-25T19:24:59Z

proto/denommetadata/genesis.proto

-  // params are all parameters for the module
-  Params params = 1 [ (gogoproto.nullable) = false ];
-}
+message GenesisState {}


if this is empty, why do we need it at all?

We should keep it as it's the cosmos-sdk standard, and in case we need to store some states in the future

zale144 · 2024-04-25T19:26:45Z

proto/hub-genesis/state.proto

-      (gogoproto.nullable) = false,
-      (gogoproto.castrepeated) = "github.com/cosmos/cosmos-sdk/types.Coins"
-    ];
+  // is_locked is a boolean that indicates if the genesis event has occured


Suggested change

// is_locked is a boolean that indicates if the genesis event has occured

// is_locked is a boolean that indicates if the genesis event has occurred

zale144 · 2024-04-25T19:27:44Z

x/denommetadata/keeper/msg_server.go

@@ -33,7 +33,12 @@ func (k msgServer) CreateDenomMetadata(
 		return nil, err
 	}

-	if !k.IsAddressPermissioned(ctx, msg.SenderAddress) {


also make it private

zale144 · 2024-04-25T19:34:56Z

x/hub-genesis/keeper/msg_server.go

@@ -27,7 +27,12 @@ func (m msgServer) TriggerGenesisEvent(goCtx context.Context, msg *types.MsgHubG
 	ctx := sdk.UnwrapSDKContext(goCtx)

 	// Get the sender and validate they are in the Allowlist
-	if !m.IsAddressInGenesisTriggererAllowList(ctx, msg.Address) {
+	accAddr, err := sdk.AccAddressFromBech32(msg.Address)


these checks could indeed be moved to a separate method. no blocker though

zale144 · 2024-04-25T19:39:58Z

x/sequencers/client/cli/query_permissions.go

+				return errors.Wrapf(err, "address format error")
+			}
+
+			res, err := queryClient.Permissions(context.Background(), &types.QueryPermissionsRequest{


Suggested change

res, err := queryClient.Permissions(context.Background(), &types.QueryPermissionsRequest{

res, err := queryClient.Permissions(cmd.Context(), &types.QueryPermissionsRequest{

zale144 · 2024-04-25T19:44:22Z

proto/sequencers/query.proto

+// method.
+message QueryPermissionsResponse {
+  // permissions defines the permissions for the given address.
+  string permissions = 1;


why not repeated string permissions = 1; ?

We need to store it in KVStore, and the repeated string, which will be convert to []string can not be Marshal to bytes in codec. This's the reason I make a wrapper struct for that

zale144 · 2024-04-25T20:03:05Z

x/denommetadata/keeper/grpc_query.go

-	params := q.GetParams(ctx)
-	return &types.QueryParamsResponse{Params: params}, nil
-}
-
 // IBCDenomByDenomTrace returns IBC denom base on denom trace
 func (q Querier) IBCDenomByDenomTrace(


I know it's not related, but this should be covered by a test. Maybe leave a TODO, this PR is already big enough.

zale144 · 2024-04-25T20:49:12Z

x/sequencers/proposal_handler.go

@@ -0,0 +1,57 @@
+package sequencers
+


be nice to write a test for these

zale144 · 2024-04-25T20:58:07Z

x/sequencers/proposal_handler.go

+func NewUpdatePermissionProposalHandler(k *keeper.Keeper) govtypes.Handler {
+	return func(ctx sdk.Context, content govtypes.Content) error {
+		switch c := content.(type) {
+		case *types.GrantPermissionsProposal:
+			return HandleGrantPermissionsProposal(ctx, k, c)
+		case *types.RevokePermissionsProposal:
+			return HandleRevokePermissionsProposal(ctx, k, c)
+		default:
+			return errorsmod.Wrapf(sdkerrors.ErrUnknownRequest, "unrecognized permissions proposal content type: %T", c)
+		}
+	}
+}
+
+// HandleGrantPermissionsProposal is a handler for executing a grant permissions proposal
+func HandleGrantPermissionsProposal(ctx sdk.Context, k *keeper.Keeper, p *types.GrantPermissionsProposal) error {
+	if err := p.ValidateBasic(); err != nil {
+		return err
+	}
+
+	for _, addrPerms := range p.AddressPermissions {
+		accAddr, err := sdk.AccAddressFromBech32(addrPerms.Address)
+		if err != nil {
+			return err
+		}
+
+		k.GrantPermissions(ctx, accAddr, addrPerms.PermissionList)
+	}
+	return nil
+}
+
+// HandleRevokePermissionsProposal is a handler for executing a revoke permissions proposal
+func HandleRevokePermissionsProposal(ctx sdk.Context, k *keeper.Keeper, p *types.RevokePermissionsProposal) error {
+	if err := p.ValidateBasic(); err != nil {
+		return err
+	}
+
+	for _, addrPerms := range p.AddressPermissions {
+		accAddr, err := sdk.AccAddressFromBech32(addrPerms.Address)
+		if err != nil {
+			return err
+		}
+
+		k.RevokePermissions(ctx, accAddr, addrPerms.PermissionList)
+	}
+	return nil
+}


There is some repetition here - could make it DRYer like so:

Suggested change

func NewUpdatePermissionProposalHandler(k *keeper.Keeper) govtypes.Handler {

return func(ctx sdk.Context, content govtypes.Content) error {

switch c := content.(type) {

case *types.GrantPermissionsProposal:

return HandleGrantPermissionsProposal(ctx, k, c)

case *types.RevokePermissionsProposal:

return HandleRevokePermissionsProposal(ctx, k, c)

default:

return errorsmod.Wrapf(sdkerrors.ErrUnknownRequest, "unrecognized permissions proposal content type: %T", c)

}

}

}

// HandleGrantPermissionsProposal is a handler for executing a grant permissions proposal

func HandleGrantPermissionsProposal(ctx sdk.Context, k *keeper.Keeper, p *types.GrantPermissionsProposal) error {

if err := p.ValidateBasic(); err != nil {

return err

}

for _, addrPerms := range p.AddressPermissions {

accAddr, err := sdk.AccAddressFromBech32(addrPerms.Address)

if err != nil {

return err

}

k.GrantPermissions(ctx, accAddr, addrPerms.PermissionList)

}

return nil

}

// HandleRevokePermissionsProposal is a handler for executing a revoke permissions proposal

func HandleRevokePermissionsProposal(ctx sdk.Context, k *keeper.Keeper, p *types.RevokePermissionsProposal) error {

if err := p.ValidateBasic(); err != nil {

return err

}

for _, addrPerms := range p.AddressPermissions {

accAddr, err := sdk.AccAddressFromBech32(addrPerms.Address)

if err != nil {

return err

}

k.RevokePermissions(ctx, accAddr, addrPerms.PermissionList)

}

return nil

}

func NewUpdatePermissionProposalHandler(k *keeper.Keeper) govtypes.Handler {

return func(ctx sdk.Context, content govtypes.Content) error {

if err := content.ValidateBasic(); err != nil {

return err

}

switch c := content.(type) {

case *types.GrantPermissionsProposal:

return HandlePermissionsProposal(ctx, c.AddressPermissions, k.GrantPermissions)

case *types.RevokePermissionsProposal:

return HandlePermissionsProposal(ctx, c.AddressPermissions, k.RevokePermissions)

default:

return errorsmod.Wrapf(sdkerrors.ErrUnknownRequest, "unrecognized permissions proposal content type: %T", c)

}

}

}

// action can be grant or revoke

type actionFn func(ctx sdk.Context, accAddr sdk.AccAddress, permList types.PermissionList)

// HandlePermissionsProposal is a handler for executing a permissions proposal

func HandlePermissionsProposal(ctx sdk.Context, perms []types.AddressPermissions, action actionFn) error {

for _, perm := range perms {

accAddr, err := sdk.AccAddressFromBech32(perm.Address)

if err != nil {

return err

}

action(ctx, accAddr, perm.PermissionList)

}

return nil

}

zale144

Good work. Just a couple of minor things that could maybe be either resolved now, or left as TODOs

danwt · 2024-04-25T21:23:46Z

x/sequencers/keeper/permission.go

+	newPerms := slices.DeleteFunc(permissionList.Permissions, func(perm string) bool {
+		return slices.Contains(revokePermList.Permissions, perm)
+	})


n^2 dos vector?

I don't think this could make the chain receive dos attack, currently we just have vesting, hubgenesis, and denommetadata module that require permissions. This won't affect anything

danwt · 2024-04-25T21:31:33Z

x/sequencers/types/permission.go

+	if !p.Equal(NewPermissionsList(perms)) {
+		return fmt.Errorf("PermissionList is not sorted yet")
+	}


why do we require sorted? should have a comment explaining

can remove sorting, thank you

Move all permissions address from denommetadata, hubgenesis to sequen…

6312884

…cers

trinitys7 requested a review from a team as a code owner April 25, 2024 06:34

lint

3ee3e5a