[CORE-794] enable authz module to allow granting privileges #960
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,12 +2,17 @@ package msgs | |
|
|
||
| import ( | ||
| sdk "github.com/cosmos/cosmos-sdk/types" | ||
| "github.com/cosmos/cosmos-sdk/x/authz" | ||
| gov "github.com/cosmos/cosmos-sdk/x/gov/types/v1" | ||
| ) | ||
|
|
||
| var ( | ||
| // NestedMsgSamples are msgs that have can include other arbitrary messages inside. | ||
| NestedMsgSamples = map[string]sdk.Msg{ | ||
| // authz | ||
|
There was a problem hiding this comment. so this PR also updates nested messages verification (see this commit). see
|
||
| "/cosmos.authz.v1beta1.MsgExec": &authz.MsgExec{}, | ||
| "/cosmos.authz.v1beta1.MsgExecResponse": nil, | ||
|
|
||
| // gov | ||
| "/cosmos.gov.v1.MsgSubmitProposal": &gov.MsgSubmitProposal{}, | ||
| "/cosmos.gov.v1.MsgSubmitProposalResponse": nil, | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,9 @@ | |
| }, | ||
| "accounts": [] | ||
| }, | ||
| "authz": { | ||
| "authorization": [] | ||
| }, | ||
| "bank": { | ||
| "params": { | ||
| "send_enabled": [], | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ package v_4_0_0 | |
| import ( | ||
| store "cosmossdk.io/store/types" | ||
| circuittypes "cosmossdk.io/x/circuit/types" | ||
| authzkeeper "github.com/cosmos/cosmos-sdk/x/authz/keeper" | ||
| icacontrollertypes "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/controller/types" | ||
| "github.com/dydxprotocol/v4-chain/protocol/app/upgrades" | ||
| ) | ||
|
|
@@ -21,6 +22,9 @@ var Upgrade = upgrades.Upgrade{ | |
|
|
||
| // Add new ICA stores that are needed by ICA host types as of v8. | ||
| icacontrollertypes.StoreKey, | ||
|
|
||
| // Add authz module to allow granting arbitrary privileges from one account to another acocunt. | ||
|
There was a problem hiding this comment. Is there anything else needed in the upgrade to initialize There was a problem hiding this comment. yeah I think this should be empty. there is currently no authorizations since it was not enabled previously Each |
||
| authzkeeper.StoreKey, | ||
| }, | ||
| }, | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,73 +2,99 @@ package ante | |
|
|
||
| import ( | ||
| "fmt" | ||
| "strings" | ||
|
|
||
| sdk "github.com/cosmos/cosmos-sdk/types" | ||
| authz "github.com/cosmos/cosmos-sdk/x/authz" | ||
| gov "github.com/cosmos/cosmos-sdk/x/gov/types/v1" | ||
| "github.com/dydxprotocol/v4-chain/protocol/lib/metrics" | ||
| ) | ||
|
|
||
| const DYDX_MSG_PREFIX = "/dydxprotocol" | ||
|
There was a problem hiding this comment. mega nit: might be better to use the AppName |
||
|
|
||
| // IsNestedMsg returns true if the given msg is a nested msg. | ||
| func IsNestedMsg(msg sdk.Msg) bool { | ||
| switch msg.(type) { | ||
|
There was a problem hiding this comment. Question: do we need There was a problem hiding this comment. good callout. I think so since it also wraps other messages. Let's add this in a separate PR since it's unrelated. I also have less context on delay messages so maybe one of the OTE eng can pick this up? |
||
| case | ||
| // ------- CosmosSDK default modules | ||
| // authz | ||
| *authz.MsgExec, | ||
| // gov | ||
| *gov.MsgSubmitProposal: | ||
| return true | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| // IsDydxMsg returns true if the given msg is a dYdX custom msg. | ||
| func IsDydxMsg(msg sdk.Msg) bool { | ||
| return strings.HasPrefix(sdk.MsgTypeURL(msg), DYDX_MSG_PREFIX) | ||
| } | ||
|
|
||
| // ValidateNestedMsg returns err if the given msg is an invalid nested msg. | ||
| func ValidateNestedMsg(msg sdk.Msg) error { | ||
| if !IsNestedMsg(msg) { | ||
| return fmt.Errorf("not a nested msg") | ||
| } | ||
|
|
||
| // Check that the inner msgs are valid. | ||
| if err := validateInnerMsg(msg); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return nil // is valid nested msg. | ||
| } | ||
|
|
||
| // validateInnerMsg returns err if the given inner msgs contain an invalid msg. | ||
| func validateInnerMsg(msg sdk.Msg) error { | ||
| // Get inner msgs. | ||
| innerMsgs, err := getInnerMsgs(msg) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| for _, inner := range innerMsgs { | ||
| // 1. unsupported msgs. | ||
| if IsUnsupportedMsg(inner) { | ||
| return fmt.Errorf("Invalid nested msg: unsupported msg type") | ||
| } | ||
|
|
||
| // 2. app-injected msgs. | ||
| if IsAppInjectedMsg(inner) { | ||
| return fmt.Errorf("Invalid nested msg: app-injected msg type") | ||
| } | ||
|
|
||
| // 3. double-nested msgs. | ||
| if IsNestedMsg(inner) { | ||
| return fmt.Errorf("Invalid nested msg: double-nested msg type") | ||
| } | ||
|
|
||
| // 4.Reject nested dydxprotocol messages in `MsgExec`. | ||
jayy04 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if _, ok := msg.(*authz.MsgExec); ok { | ||
| metrics.IncrCountMetricWithLabels( | ||
jayy04 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| metrics.Ante, | ||
| metrics.MsgExec, | ||
| metrics.GetLabelForStringValue(metrics.InnerMsg, sdk.MsgTypeURL(inner)), | ||
| ) | ||
| if IsDydxMsg(inner) { | ||
|
There was a problem hiding this comment. Does this mean we don't allow Is there any context/discussion behind this? If so could you document briefly? There was a problem hiding this comment. ah there was a comment discussion in the authz one pager that got resolved. I added a short section in the doc saying that dydx messages will be explicitly disallowed for now and an allowlist can be maintained in the future if there are use cases. |
||
| return fmt.Errorf("Invalid nested msg for MsgExec: dydx msg type") | ||
| } | ||
| } | ||
|
|
||
| // For "internal msgs", we allow them, because they are designed to be nested. | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // getInnerMsgs returns the inner msgs of the given msg. | ||
| func getInnerMsgs(msg sdk.Msg) ([]sdk.Msg, error) { | ||
| switch msg := msg.(type) { | ||
| case | ||
| *gov.MsgSubmitProposal: | ||
jayy04 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return msg.GetMsgs() | ||
| case *authz.MsgExec: | ||
| return msg.GetMessages() | ||
| default: | ||
| return nil, fmt.Errorf("unsupported msg type: %T", msg) | ||
| } | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weird this one is not set up in the same way... no
authztypes?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah not sure how
authzwas bootstrapped initially, but there is noauthz/typesand the key is underkeeper/keys.go😕There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looked into this quickly.
typesgot removed in this PR. still not sure why though