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

[CORE-794] enable authz module to allow granting privileges #960

Merged
merged 12 commits into from
Jan 25, 2024
Merged

[CORE-794] enable authz module to allow granting privileges #960

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
01ef875
[CORE-794] enable authz module to allow granting privileges
jayy04 Jan 11, 2024
3cc63a5
fix tests
jayy04 Jan 12, 2024
247a97f
fix simulation
jayy04 Jan 12, 2024
e1b924f
update ante
jayy04 Jan 12, 2024
030a4be
e2e tests
jayy04 Jan 12, 2024
76adfd6
more tests
jayy04 Jan 12, 2024
287efa5
comments
jayy04 Jan 16, 2024
46e0f31
Merge branch 'main' into jy/core-794
jayy04 Jan 21, 2024
9ff75f7
disallow all dydx messages
jayy04 Jan 21, 2024
902985d
fix constant
jayy04 Jan 21, 2024
697482c
Merge branch 'main' into jy/core-794
jayy04 Jan 24, 2024
07ce065
comments
jayy04 Jan 24, 2024
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
35 changes: 31 additions & 4 deletions protocol/app/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@ import (
authkeeper "github.com/cosmos/cosmos-sdk/x/auth/keeper"
authtx "github.com/cosmos/cosmos-sdk/x/auth/tx"
authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
authz "github.com/cosmos/cosmos-sdk/x/authz"
authzkeeper "github.com/cosmos/cosmos-sdk/x/authz/keeper"
authzmodule "github.com/cosmos/cosmos-sdk/x/authz/module"
"github.com/cosmos/cosmos-sdk/x/bank"
bankkeeper "github.com/cosmos/cosmos-sdk/x/bank/keeper"
banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
Expand Down Expand Up @@ -234,6 +237,7 @@ type App struct {

// keepers
AccountKeeper authkeeper.AccountKeeper
AuthzKeeper authzkeeper.Keeper
BankKeeper bankkeeper.Keeper
CapabilityKeeper *capabilitykeeper.Keeper
StakingKeeper *stakingkeeper.Keeper
Expand Down Expand Up @@ -346,10 +350,20 @@ func New(
bApp.SetTxEncoder(txConfig.TxEncoder())

keys := storetypes.NewKVStoreKeys(
authtypes.StoreKey, banktypes.StoreKey, stakingtypes.StoreKey, crisistypes.StoreKey,
distrtypes.StoreKey, slashingtypes.StoreKey,
govtypes.StoreKey, paramstypes.StoreKey, consensusparamtypes.StoreKey, upgradetypes.StoreKey, feegrant.StoreKey,
ibcexported.StoreKey, ibctransfertypes.StoreKey,
authtypes.StoreKey,
authzkeeper.StoreKey,
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah not sure how authz was bootstrapped initially, but there is no authz/types and the key is under keeper/keys.go 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looked into this quickly. types got removed in this PR. still not sure why though

banktypes.StoreKey,
stakingtypes.StoreKey,
crisistypes.StoreKey,
distrtypes.StoreKey,
slashingtypes.StoreKey,
govtypes.StoreKey,
paramstypes.StoreKey,
consensusparamtypes.StoreKey,
upgradetypes.StoreKey,
feegrant.StoreKey,
ibcexported.StoreKey,
ibctransfertypes.StoreKey,
ratelimitmoduletypes.StoreKey,
icacontrollertypes.StoreKey,
icahosttypes.StoreKey,
Expand Down Expand Up @@ -429,6 +443,14 @@ func New(
sdk.GetConfig().GetBech32AccountAddrPrefix(),
lib.GovModuleAddress.String(),
)

app.AuthzKeeper = authzkeeper.NewKeeper(
runtime.NewKVStoreService(keys[authzkeeper.StoreKey]),
appCodec,
Comment on lines +451 to +452
Copy link
Contributor

Choose a reason for hiding this comment

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

the order of these params is also reversed vs most NewKeeper methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah authz always have store key before codec, not sure how it was bootstrapped

app.MsgServiceRouter(),
app.AccountKeeper,
)

app.BankKeeper = bankkeeper.NewBaseKeeper(
appCodec,
runtime.NewKVStoreService(keys[banktypes.StoreKey]),
Expand Down Expand Up @@ -987,6 +1009,7 @@ func New(
),
auth.NewAppModule(appCodec, app.AccountKeeper, nil, app.getSubspace(authtypes.ModuleName)),
bank.NewAppModule(appCodec, app.BankKeeper, app.AccountKeeper, app.getSubspace(banktypes.ModuleName)),
authzmodule.NewAppModule(appCodec, app.AuthzKeeper, app.AccountKeeper, app.BankKeeper, app.interfaceRegistry),
capability.NewAppModule(appCodec, *app.CapabilityKeeper, false),
feegrantmodule.NewAppModule(appCodec, app.AccountKeeper, app.BankKeeper, app.FeeGrantKeeper, app.interfaceRegistry),
crisis.NewAppModule(app.CrisisKeeper, skipGenesisInvariants, app.getSubspace(crisistypes.ModuleName)),
Expand Down Expand Up @@ -1048,6 +1071,7 @@ func New(
// NOTE: staking module is required if HistoricalEntries param > 0
app.ModuleManager.SetOrderBeginBlockers(
blocktimemoduletypes.ModuleName, // Must be first
authz.ModuleName, // Delete expired grants.
epochsmoduletypes.ModuleName,
capabilitytypes.ModuleName,
distrtypes.ModuleName,
Expand Down Expand Up @@ -1116,6 +1140,7 @@ func New(
rewardsmoduletypes.ModuleName,
epochsmoduletypes.ModuleName,
delaymsgmoduletypes.ModuleName,
authz.ModuleName, // No-op.
blocktimemoduletypes.ModuleName, // Must be last
)

Expand Down Expand Up @@ -1157,6 +1182,7 @@ func New(
rewardsmoduletypes.ModuleName,
sendingmoduletypes.ModuleName,
delaymsgmoduletypes.ModuleName,
authz.ModuleName,
)

// NOTE: by default, set migration order here to be the same as init genesis order,
Expand Down Expand Up @@ -1194,6 +1220,7 @@ func New(
rewardsmoduletypes.ModuleName,
sendingmoduletypes.ModuleName,
delaymsgmoduletypes.ModuleName,
authz.ModuleName,

// Auth must be migrated after staking.
authtypes.ModuleName,
Expand Down
2 changes: 2 additions & 0 deletions protocol/app/app_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
tmproto "github.com/cometbft/cometbft/proto/tendermint/types"
"github.com/cosmos/cosmos-sdk/types/module"
"github.com/cosmos/cosmos-sdk/x/auth"
authzmodule "github.com/cosmos/cosmos-sdk/x/authz/module"
"github.com/cosmos/cosmos-sdk/x/bank"
"github.com/cosmos/cosmos-sdk/x/consensus"
"github.com/cosmos/cosmos-sdk/x/crisis"
Expand Down Expand Up @@ -201,6 +202,7 @@ func TestModuleBasics(t *testing.T) {
upgrade.AppModuleBasic{},
transfer.AppModuleBasic{},
consensus.AppModuleBasic{},
authzmodule.AppModuleBasic{},

// Custom modules
pricesmodule.AppModuleBasic{},
Expand Down
2 changes: 2 additions & 0 deletions protocol/app/basic_manager/basic_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"cosmossdk.io/x/upgrade"
"github.com/cosmos/cosmos-sdk/types/module"
"github.com/cosmos/cosmos-sdk/x/auth"
authzmodule "github.com/cosmos/cosmos-sdk/x/authz/module"
"github.com/cosmos/cosmos-sdk/x/bank"
"github.com/cosmos/cosmos-sdk/x/consensus"
"github.com/cosmos/cosmos-sdk/x/crisis"
Expand Down Expand Up @@ -71,6 +72,7 @@ var (
evidence.AppModuleBasic{},
transfer.AppModuleBasic{},
consensus.AppModuleBasic{},
authzmodule.AppModuleBasic{},

// Custom modules
pricesmodule.AppModuleBasic{},
Expand Down
9 changes: 9 additions & 0 deletions protocol/app/msgs/all_msgs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,15 @@ var (
"/cosmos.auth.v1beta1.ModuleCredential": {},
"/cosmos.auth.v1beta1.MsgUpdateParams": {},

// authz
"/cosmos.authz.v1beta1.GenericAuthorization": {},
"/cosmos.authz.v1beta1.MsgExec": {},
"/cosmos.authz.v1beta1.MsgExecResponse": {},
"/cosmos.authz.v1beta1.MsgGrant": {},
"/cosmos.authz.v1beta1.MsgGrantResponse": {},
"/cosmos.authz.v1beta1.MsgRevoke": {},
"/cosmos.authz.v1beta1.MsgRevokeResponse": {},

// bank
"/cosmos.bank.v1beta1.MsgMultiSend": {},
"/cosmos.bank.v1beta1.MsgMultiSendResponse": {},
Expand Down
5 changes: 5 additions & 0 deletions protocol/app/msgs/nested_msgs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

Haven't dug into how nested messages are verified - will someone be able to execute an otherwise disallowed messages as a nested message under MsgExec? Could you reply to @ttl33 's relevant question here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so this PR also updates nested messages verification (see this commit).

see validateInnerMsg for more details . Essentially we disallow inner messages to be

  1. unsupported (some ICA controller messages)
  2. app injected
  3. another nested message

"/cosmos.authz.v1beta1.MsgExec": &authz.MsgExec{},
"/cosmos.authz.v1beta1.MsgExecResponse": nil,

// gov
"/cosmos.gov.v1.MsgSubmitProposal": &gov.MsgSubmitProposal{},
"/cosmos.gov.v1.MsgSubmitProposalResponse": nil,
Expand Down
4 changes: 4 additions & 0 deletions protocol/app/msgs/nested_msgs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ import (

func TestNestedMsgs_Key(t *testing.T) {
expectedMsgs := []string{
// authz
"/cosmos.authz.v1beta1.MsgExec",
"/cosmos.authz.v1beta1.MsgExecResponse",

// gov
"/cosmos.gov.v1.MsgSubmitProposal",
"/cosmos.gov.v1.MsgSubmitProposalResponse",
Expand Down
8 changes: 8 additions & 0 deletions protocol/app/msgs/normal_msgs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
evidence "cosmossdk.io/x/evidence/types"
feegrant "cosmossdk.io/x/feegrant"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/x/authz"
bank "github.com/cosmos/cosmos-sdk/x/bank/types"
crisis "github.com/cosmos/cosmos-sdk/x/crisis/types"
distr "github.com/cosmos/cosmos-sdk/x/distribution/types"
Expand All @@ -27,6 +28,13 @@ var (
"/cosmos.auth.v1beta1.ModuleAccount": nil,
"/cosmos.auth.v1beta1.ModuleCredential": nil,

// authz
"/cosmos.authz.v1beta1.GenericAuthorization": nil,
"/cosmos.authz.v1beta1.MsgGrant": &authz.MsgGrant{},
"/cosmos.authz.v1beta1.MsgGrantResponse": nil,
"/cosmos.authz.v1beta1.MsgRevoke": &authz.MsgRevoke{},
"/cosmos.authz.v1beta1.MsgRevokeResponse": nil,

// bank
"/cosmos.bank.v1beta1.MsgMultiSend": &bank.MsgMultiSend{},
"/cosmos.bank.v1beta1.MsgMultiSendResponse": nil,
Expand Down
7 changes: 7 additions & 0 deletions protocol/app/msgs/normal_msgs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,13 @@ func TestNormalMsgs_Key(t *testing.T) {
"/cosmos.auth.v1beta1.ModuleAccount",
"/cosmos.auth.v1beta1.ModuleCredential",

// authz
"/cosmos.authz.v1beta1.GenericAuthorization",
"/cosmos.authz.v1beta1.MsgGrant",
"/cosmos.authz.v1beta1.MsgGrantResponse",
"/cosmos.authz.v1beta1.MsgRevoke",
"/cosmos.authz.v1beta1.MsgRevokeResponse",

// bank
"/cosmos.bank.v1beta1.MsgMultiSend",
"/cosmos.bank.v1beta1.MsgMultiSendResponse",
Expand Down
6 changes: 2 additions & 4 deletions protocol/app/msgs/unregistered_msgs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,8 @@ var (
// UnregisteredMsgs are msgs that should not be registered with the app.
UnregisteredMsgs = map[string]struct{}{
// authz
"/cosmos.authz.v1.MsgExec": {},
"/cosmos.authz.v1.MsgExecResponse": {},
"/cosmos.authz.v1beta1.MsgExec": {},
"/cosmos.authz.v1beta1.MsgExecResponse": {},
"/cosmos.authz.v1.MsgExec": {},
"/cosmos.authz.v1.MsgExecResponse": {},

// group
"/cosmos.group.v1.MsgSubmitProposal": {},
Expand Down
2 changes: 0 additions & 2 deletions protocol/app/msgs/unregistered_msgs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,6 @@ func TestUnregisteredMsgs_Key(t *testing.T) {
// authz
"/cosmos.authz.v1.MsgExec",
"/cosmos.authz.v1.MsgExecResponse",
"/cosmos.authz.v1beta1.MsgExec",
"/cosmos.authz.v1beta1.MsgExecResponse",

// group
"/cosmos.group.v1.MsgSubmitProposal",
Expand Down
7 changes: 5 additions & 2 deletions protocol/app/simulation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,15 @@ package app_test
import (
"encoding/json"
"fmt"
dbm "github.com/cosmos/cosmos-db"
icatypes "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/types"
"io"
"math/rand"
"os"
"testing"
"time"

dbm "github.com/cosmos/cosmos-db"
icatypes "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/types"

"cosmossdk.io/log"
"cosmossdk.io/math"
"cosmossdk.io/store"
Expand All @@ -33,6 +34,7 @@ import (
authsims "github.com/cosmos/cosmos-sdk/x/auth/simulation"
authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
vestingtypes "github.com/cosmos/cosmos-sdk/x/auth/vesting/types"
authz "github.com/cosmos/cosmos-sdk/x/authz"
banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
distributiontypes "github.com/cosmos/cosmos-sdk/x/distribution/types"
govtypes "github.com/cosmos/cosmos-sdk/x/gov/types"
Expand Down Expand Up @@ -105,6 +107,7 @@ func (app *SimApp) SimulationManager() *module.SimulationManager {
var genesisModuleOrder = []string{
authtypes.ModuleName,
banktypes.ModuleName,
authz.ModuleName,
capabilitytypes.ModuleName,
feegranttypes.ModuleName,
govtypes.ModuleName,
Expand Down
3 changes: 3 additions & 0 deletions protocol/app/testdata/default_genesis_state.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@
},
"accounts": []
},
"authz": {
"authorization": []
},
"bank": {
"params": {
"send_enabled": [],
Expand Down
4 changes: 4 additions & 0 deletions protocol/app/upgrades/v4.0.0/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
icahosttypes "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/host/types"
"github.com/dydxprotocol/v4-chain/protocol/app/upgrades"
Expand All @@ -23,6 +24,9 @@ var Upgrade = upgrades.Upgrade{
// Add new ICA stores that are needed by ICA host types as of v8.
icacontrollertypes.StoreKey,
icahosttypes.StoreKey,

// Add authz module to allow granting arbitrary privileges from one account to another acocunt.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there anything else needed in the upgrade to initialize authz state? Is this inherently left empty with no Authorization?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I think this should be empty. there is currently no authorizations since it was not enabled previously

Each Authorization consists of (granter, grantee, msgURL) and permissions needs to be granted explicitly by the granter

authzkeeper.StoreKey,
},
},
}
5 changes: 5 additions & 0 deletions protocol/lib/ante/nested_msg.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"fmt"

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"
)

Expand All @@ -12,6 +13,8 @@ func IsNestedMsg(msg sdk.Msg) bool {
switch msg.(type) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: do we need MsgDelayMsg here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Expand Down Expand Up @@ -45,6 +48,8 @@ func getInnerMsgs(msg sdk.Msg) ([]sdk.Msg, error) {
case
*gov.MsgSubmitProposal:
return msg.GetMsgs()
case *authz.MsgExec:
return msg.GetMessages()
default:
return nil, fmt.Errorf("unsupported msg type: %T", msg)
}
Expand Down
12 changes: 12 additions & 0 deletions protocol/lib/ante/nested_msg_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,18 @@ func TestValidateNestedMsg(t *testing.T) {
msg: testmsgs.MsgSubmitProposalWithDoubleNestedInner,
expectedErr: invalidInnerMsgErr_Nested,
},
"Invalid MsgExec: unsupported inner msg": {
Copy link
Contributor

Choose a reason for hiding this comment

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

should we add a testcase for the dydx custom msg types?

msg: &testmsgs.MsgExecWithUnsupportedInner,
expectedErr: invalidInnerMsgErr_Unsupported,
},
"Invalid MsgExec: app-injected inner msg": {
msg: &testmsgs.MsgExecWithAppInjectedInner,
expectedErr: invalidInnerMsgErr_AppInjected,
},
"Invalid MsgExec: double-nested inner msg": {
msg: &testmsgs.MsgExecWithDoubleNestedInner,
expectedErr: invalidInnerMsgErr_Nested,
},
"Valid: empty inner msg": {
msg: testmsgs.MsgSubmitProposalWithEmptyInner,
expectedErr: nil,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: should we add test cases for IsDydxMsg method?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree this would be valuable to have 👍

Expand Down
3 changes: 3 additions & 0 deletions protocol/scripts/genesis/sample_pregenesis.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@
"tx_size_cost_per_byte": "10"
}
},
"authz": {
"authorization": []
},
"bank": {
"balances": [
{
Expand Down
Loading
Loading