Msg Authorization ADR #5235
Msg Authorization ADR #5235
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5235 +/- ##
==========================================
+ Coverage 53.51% 53.55% +0.04%
==========================================
Files 290 290
Lines 17770 17719 -51
==========================================
- Hits 9509 9489 -20
+ Misses 7511 7489 -22
+ Partials 750 741 -9 |
| Grantee sdk.AccAddress `json:"grantee"` | ||
| Capability Capability `json:"capability"` | ||
| // Expiration specifies the expiration time of the grant | ||
| Expiration time.Time `json:"expiration"` |
There was a problem hiding this comment.
Perhaps we should piggy back off of what fee delegation provides in that you can expire at a height or time. We could use the concrete type defined in that PR.
There was a problem hiding this comment.
I'm fine using the existing struct but I do question the utility of both height and time. It seems like adding height introduces other complexities when exporting to genesis. I commented about this in the fee delegation PR as well. Basically why is it desired to support both @alexanderbez or would just time be sufficient?
There was a problem hiding this comment.
IMHO time is sufficient and simpler. In that case, we should just use int64
| } | ||
| ``` | ||
|
|
||
| ### MsgRevoke |
There was a problem hiding this comment.
The UX is a bit confusing here IMHO. Capabilities are granted, but messages are revoked? What if I wanted to revoke an entire capability instead of doing it message by message?
There was a problem hiding this comment.
So the reason for this is because capabilities are stored based on the sdk.Msg type they refer to. That is the key that needs to be revoked. Would it make it clearer if MsgRevoke had a field CapabilityMsgType like this?
type MsgRevoke struct {
Granter sdk.AccAddress `json:"granter"`
Grantee sdk.AccAddress `json:"grantee"`
// CapabilityMsgType is the type of sdk.Msg that the revoked Capability refers to.
// i.e. this is what `Capability.MsgType()` returns
CapabilityMsgType sdk.Msg `json:"capability_msg_type"`
}| `DispatchActions attempts to execute the provided messages via capability | ||
| grants from the message signer to the grantee. | ||
|
|
||
| ### `Grant(ctx sdk.Context, grantee sdk.AccAddress, granter sdk.AccAddress, capability Capability, expiration time.Time)` |
There was a problem hiding this comment.
markdown linting is needed here.
| // MsgExecDelegated attempts to execute the provided messages using | ||
| // capabilities granted to the grantee. Each message should have only | ||
| // one signer corresponding to the granter of the capability. | ||
| type MsgExecDelegated struct { |
There was a problem hiding this comment.
What this pseudo-spec fails to mention, yet should, is how this is wired together. It should be noted that when a client wishes to send a tx with a delegated message(s), they need to wrap it with this message type and that the client logic (CLI and REST) should do this automatically for the client via some flags/params.
There was a problem hiding this comment.
Adding a --send-as flag to the docs
There was a problem hiding this comment.
Thanks @aaronc. Left some feedback. The ADR also needs to be added in the TOC section in docs/architecture/README.MD.
| We will create a module named `msg_authorization` | ||
|
|
||
| The `msg_authorization` module provides support for granting arbitrary capabilities |
There was a problem hiding this comment.
We will create a module named msg_authorization. The msg_authorization module provides ...
|
|
||
| - 2019-11-06: Initial ADR | ||
|
|
||
| ## Context |
There was a problem hiding this comment.
The context should state the (concise) motivation for why we believe this is needed.
| ```go | ||
| type Capability interface { | ||
| // MsgType returns the type of Msg's that this capability can accept | ||
| MsgType() sdk.Msg |
There was a problem hiding this comment.
The more I look at this contract in the interface, the more is kinda bugs me. I feel like this should be MsgType() string. Since the sdk.Msg interface already has a Type() string method. This way we can directly compare against const values and not type match.
| } | ||
|
|
||
| func (cap SendCapability) MsgType() sdk.Msg { | ||
| return bank.MsgSend{} |
There was a problem hiding this comment.
| return bank.MsgSend{} | |
| return bank.TypeMsgSend |
| } | ||
|
|
||
| func (cap SendCapability) Accept(msg sdk.Msg, block abci.Header) (allow bool, updated Capability, delete bool) { | ||
| switch msg := msg.(type) { |
There was a problem hiding this comment.
see above. just switch on the string value and use const.
| ``` | ||
| #### `tx grant <grantee> <capability> --from <granter>` | ||
|
|
||
| This CLI command will send a `MsgGrant` tx. `capability` should be encoded as |
There was a problem hiding this comment.
We can expand a bit upon this I think. Just add a bit more detail.
|
|
||
| #### `tx revoke <grantee> <capability-msg-type> --from <granter>` | ||
|
|
||
| This CLI command will send a `MsgRevoke` tx. `capability-msg-type` should be encoded as |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
@alexanderbez @jackzampolin this PR points to our latest spec on this "generic msg delegation/authorization" functionality so that we have a place to discuss the architectural details in preparation for next time we meet. This supersedes the previous spec although this module doesn't really have any substantive differences from that spec. I know we want something different from
msg_delegation. Once I've got a clear idea, I'll rename it and update the PR.