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

feat: add message index event attribute to authz message execution #12668

Merged
merged 11 commits into from
Jul 21, 2022
Merged

feat: add message index event attribute to authz message execution #12668

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
60a8749
updates
alexanderbez Jul 21, 2022
d22020d
updates
alexanderbez Jul 21, 2022
cb098e1
updates
alexanderbez Jul 21, 2022
ea90ff9
updates
alexanderbez Jul 21, 2022
2654290
Merge branch 'main' into bez/authz-msg-events-index
alexanderbez Jul 21, 2022
535a317
updates
alexanderbez Jul 21, 2022
694fa55
Merge branch 'bez/authz-msg-events-index' of github.com:cosmos/cosmos…
alexanderbez Jul 21, 2022
69a499d
updates
alexanderbez Jul 21, 2022
27ff388
updates
alexanderbez Jul 21, 2022
8824e7d
Merge branch 'main' into bez/authz-msg-events-index
julienrbrt Jul 21, 2022
2e45553
Merge branch 'main' into bez/authz-msg-events-index
julienrbrt Jul 21, 2022
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 @@ -47,6 +47,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

### Improvements

* [#12668](https://github.com/cosmos/cosmos-sdk/pull/12668) Add `authz_msg_index` event attribute to message events emitted when executing via `MsgExec` through `x/authz`.
* [#12634](https://github.com/cosmos/cosmos-sdk/pull/12634) Move `sdk.Dec` to math package.
* [#12596](https://github.com/cosmos/cosmos-sdk/pull/12596) Remove all imports of the non-existent gogo/protobuf v1.3.3 to ease downstream use and go workspaces.
* [#12589](https://github.com/cosmos/cosmos-sdk/pull/12589) Allow zero gas in simulation mode.
Expand Down
19 changes: 15 additions & 4 deletions x/authz/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,11 @@ package keeper

import (
"fmt"
"strconv"
"time"

"github.com/gogo/protobuf/proto"

abci "github.com/tendermint/tendermint/abci/types"
"github.com/tendermint/tendermint/libs/log"

"github.com/cosmos/cosmos-sdk/baseapp"
Expand Down Expand Up @@ -84,14 +85,17 @@ func (k Keeper) update(ctx sdk.Context, grantee sdk.AccAddress, granter sdk.AccA
func (k Keeper) DispatchActions(ctx sdk.Context, grantee sdk.AccAddress, msgs []sdk.Msg) ([][]byte, error) {
results := make([][]byte, len(msgs))
now := ctx.BlockTime()

for i, msg := range msgs {
signers := msg.GetSigners()
if len(signers) != 1 {
return nil, authz.ErrAuthorizationNumOfSigners
}

granter := signers[0]

// if granter != grantee then check authorization.Accept, otherwise we implicitly accept.
// If granter != grantee then check authorization.Accept, otherwise we
// implicitly accept.
if !granter.Equals(grantee) {
skey := grantStoreKey(grantee, granter, sdk.MsgTypeURL(msg))

Expand All @@ -113,6 +117,7 @@ func (k Keeper) DispatchActions(ctx sdk.Context, grantee sdk.AccAddress, msgs []
if err != nil {
return nil, err
}

if resp.Delete {
err = k.DeleteGrant(ctx, grantee, granter, sdk.MsgTypeURL(msg))
} else if resp.Updated != nil {
Expand All @@ -121,6 +126,7 @@ func (k Keeper) DispatchActions(ctx sdk.Context, grantee sdk.AccAddress, msgs []
if err != nil {
return nil, err
}

if !resp.Accept {
return nil, sdkerrors.ErrUnauthorized
}
Expand All @@ -135,14 +141,19 @@ func (k Keeper) DispatchActions(ctx sdk.Context, grantee sdk.AccAddress, msgs []
if err != nil {
return nil, sdkerrors.Wrapf(err, "failed to execute message; message %v", msg)
}

results[i] = msgResp.Data

// emit the events from the dispatched actions
events := msgResp.Events
sdkEvents := make([]sdk.Event, 0, len(events))
for i := 0; i < len(events); i++ {
sdkEvents = append(sdkEvents, sdk.Event(events[i]))
for _, event := range events {
e := event
e.Attributes = append(e.Attributes, abci.EventAttribute{Key: "authz_msg_index", Value: strconv.Itoa(i)})
Copy link
Member

@julienrbrt julienrbrt Jul 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

If we move back to normal event, we should define this in an events.go as AttributeKeyMsgIndex or smth. Now we have this module combining TypedEvent and “Legacy” Events while the documentation state otherwise: https://docs.cosmos.network/v0.46/modules/authz/04_events.html

Copy link
Member

Choose a reason for hiding this comment

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

agere. Im more inclined to revert the changes and then move to it later but need to make a team call on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huh? Where is there typed events used here?

Copy link
Member

@julienrbrt julienrbrt Jul 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Not in your code, but here https://github.com/cosmos/cosmos-sdk/blob/main/x/authz/keeper/keeper.go#L195-L199 and there https://github.com/cosmos/cosmos-sdk/blob/main/x/authz/keeper/keeper.go#L221-L225

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahh yeah, but we're just adding an attribute here though. What do you want to change?

Copy link
Member

Choose a reason for hiding this comment

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

I was just noticing the usage of two types of events in the authz module :)


sdkEvents = append(sdkEvents, sdk.Event(e))
}

ctx.EventManager().EmitEvents(sdkEvents)
}

Expand Down
3 changes: 3 additions & 0 deletions x/authz/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,9 +316,12 @@ func (s *TestSuite) TestDispatchedEvents() {
result, err := s.authzKeeper.DispatchActions(s.ctx, granteeAddr, executeMsgs)
require.NoError(err)
require.NotNil(result)

events := s.ctx.EventManager().Events()

// get last 5 events (events that occur *after* the grant)
events = events[len(events)-5:]

requiredEvents := map[string]bool{
"coin_spent": false,
"coin_received": false,
Expand Down
5 changes: 5 additions & 0 deletions x/authz/keeper/msg_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ func (k Keeper) Grant(goCtx context.Context, msg *authz.MsgGrant) (*authz.MsgGra
if err != nil {
return nil, err
}

// create the account if it is not in account state
granteeAcc := k.authKeeper.GetAccount(ctx, grantee)
if granteeAcc == nil {
Expand All @@ -33,6 +34,7 @@ func (k Keeper) Grant(goCtx context.Context, msg *authz.MsgGrant) (*authz.MsgGra
if err != nil {
return nil, err
}

t := authorization.MsgTypeURL()
if k.router.HandlerByTypeURL(t) == nil {
return nil, sdkerrors.ErrInvalidType.Wrapf("%s doesn't exist.", t)
Expand Down Expand Up @@ -73,13 +75,16 @@ func (k Keeper) Exec(goCtx context.Context, msg *authz.MsgExec) (*authz.MsgExecR
if err != nil {
return nil, err
}

msgs, err := msg.GetMessages()
if err != nil {
return nil, err
}

results, err := k.DispatchActions(ctx, grantee, msgs)
if err != nil {
return nil, err
}

return &authz.MsgExecResponse{Results: results}, nil
}