From c28bc03248be7e526a5087b7fdc2f43ec4b77ebd Mon Sep 17 00:00:00 2001 From: Ruaridh Date: Fri, 15 May 2020 20:25:49 +0100 Subject: [PATCH] Committtee audit revisions (#510) * comments from review Co-authored-by: Sunny Aggarwal Co-authored-by: jmahess Co-authored-by: Alexander Bezobchuk * add vote methods * add draft new param change permission * add and update tests * rename ParamChangePermission * account for perms becoming invalid at a later time * add debtParam to permission * add bep3 AssetParam to permissions * add pricefeed Markets to permission * add upgrade permission * move proposal passing to the begin blocker * fix iteration bug Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com> * address todos and audit comments * add proposal examples * refactor handler to be easier to read * address review comments * update comments Co-authored-by: Kevin Davis Co-authored-by: Sunny Aggarwal Co-authored-by: jmahess Co-authored-by: Alexander Bezobchuk Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com> --- app/app.go | 4 +- .../committee_change_proposal.json | 123 +++ .../proposal_examples/upgrade_proposal.json | 12 + x/committee/abci.go | 3 + x/committee/abci_test.go | 165 +++- x/committee/alias.go | 49 +- x/committee/client/cli/tx.go | 2 +- x/committee/handler.go | 35 +- x/committee/handler_test.go | 97 +-- .../{types => keeper}/committee_test.go | 62 +- x/committee/keeper/invariants.go | 11 +- x/committee/keeper/keeper.go | 26 +- x/committee/keeper/param_permission_test.go | 239 ++++++ x/committee/keeper/proposal.go | 54 +- x/committee/keeper/proposal_test.go | 137 +++- x/committee/proposal_handler.go | 18 +- x/committee/simulation/genesis.go | 2 +- x/committee/simulation/operations.go | 6 +- x/committee/simulation/proposals.go | 4 +- x/committee/spec/04_events.md | 5 +- x/committee/types/codec.go | 7 +- x/committee/types/committee.go | 25 +- x/committee/types/expected_keepers.go | 9 + x/committee/types/genesis.go | 9 +- x/committee/types/genesis_test.go | 1 - x/committee/types/param_permissions_test.go | 741 ++++++++++++++++++ x/committee/types/permissions.go | 465 ++++++++++- x/committee/types/permissions_test.go | 68 +- 28 files changed, 2143 insertions(+), 236 deletions(-) create mode 100644 contrib/testnet-6000/genesis_examples/proposal_examples/committee_change_proposal.json create mode 100644 contrib/testnet-6000/genesis_examples/proposal_examples/upgrade_proposal.json rename x/committee/{types => keeper}/committee_test.go (73%) create mode 100644 x/committee/keeper/param_permission_test.go create mode 100644 x/committee/types/expected_keepers.go create mode 100644 x/committee/types/param_permissions_test.go diff --git a/app/app.go b/app/app.go index 5edba683..63219115 100644 --- a/app/app.go +++ b/app/app.go @@ -266,13 +266,15 @@ func NewApp(logger log.Logger, db dbm.DB, traceStore io.Writer, loadLatest bool, committeeGovRouter. AddRoute(gov.RouterKey, gov.ProposalHandler). AddRoute(params.RouterKey, params.NewParamChangeProposalHandler(app.paramsKeeper)). - AddRoute(distr.RouterKey, distr.NewCommunityPoolSpendProposalHandler(app.distrKeeper)) + AddRoute(distr.RouterKey, distr.NewCommunityPoolSpendProposalHandler(app.distrKeeper)). + AddRoute(upgrade.RouterKey, upgrade.NewSoftwareUpgradeProposalHandler(app.upgradeKeeper)) // Note: the committee proposal handler is not registered on the committee router. This means committees cannot create or update other committees. // Adding the committee proposal handler to the router is possible but awkward as the handler depends on the keeper which depends on the handler. app.committeeKeeper = committee.NewKeeper( app.cdc, keys[committee.StoreKey], committeeGovRouter, // TODO blacklist module addresses? + app.paramsKeeper, ) // create gov keeper with router diff --git a/contrib/testnet-6000/genesis_examples/proposal_examples/committee_change_proposal.json b/contrib/testnet-6000/genesis_examples/proposal_examples/committee_change_proposal.json new file mode 100644 index 00000000..782c3db4 --- /dev/null +++ b/contrib/testnet-6000/genesis_examples/proposal_examples/committee_change_proposal.json @@ -0,0 +1,123 @@ +{ + "type": "kava/CommitteeChangeProposal", + "value": { + "title": "Update Committee 0", + "description": "This is a proposal that updates committee with id 0", + "new_committee": { + "id": "0", + "description": "This committee is for adjusting parameters of the cdp system.", + "members": [ + "kava1vysxvcttv5sxzerywfjhxucazsxj0", + "kava1v9hx7argv4ezqenpddjjqctyv3ex2umndpth74" + ], + "permissions": [ + { + "type": "kava/SubParamChangePermission", + "value": { + "allowed_params": [ + { + "subspace": "cdp", + "key": "GlobalDebtLimit" + }, + { + "subspace": "cdp", + "key": "SurplusThreshold" + }, + { + "subspace": "cdp", + "key": "DebtThreshold" + }, + { + "subspace": "cdp", + "key": "DistributionFrequency" + }, + { + "subspace": "cdp", + "key": "CircuitBreaker" + }, + { + "subspace": "cdp", + "key": "CollateralParams" + }, + { + "subspace": "cdp", + "key": "DebtParam" + }, + { + "subspace": "auction", + "key": "BidDuration" + }, + { + "subspace": "auction", + "key": "IncrementSurplus" + }, + { + "subspace": "auction", + "key": "IncrementDebt" + }, + { + "subspace": "auction", + "key": "IncrementCollateral" + }, + { + "subspace": "bep3", + "key": "SupportedAssets" + }, + { + "subspace": "pricefeed", + "key": "Markets" + }, + { + "subspace": "incentive", + "key": "Active" + }, + { + "subspace": "kavadist", + "key": "Active" + } + ], + "allowed_collateral_params": [ + { + "denom": "bnb", + "liquidation_ratio": false, + "debt_limit": true, + "stability_fee": true, + "auction_size": true, + "liquidation_penalty": false, + "prefix": false, + "market_id": false, + "conversion_factor": false + } + ], + "allowed_debt_param": { + "denom": false, + "reference_asset": false, + "conversion_factor": false, + "debt_floor": false, + "savings_rate": true + }, + "allowed_asset_params": [ + { + "denom": "bnb", + "coin_id": false, + "limit": true, + "active": true + } + ], + "allowed_markets": [ + { + "market_id": "bnb:usd", + "base_asset": false, + "quote_asset": false, + "oracles": false, + "active": true + } + ] + } + } + ], + "vote_threshold": "0.750000000000000000", + "proposal_duration": "604800000000000" + } + } +} \ No newline at end of file diff --git a/contrib/testnet-6000/genesis_examples/proposal_examples/upgrade_proposal.json b/contrib/testnet-6000/genesis_examples/proposal_examples/upgrade_proposal.json new file mode 100644 index 00000000..010f7d2a --- /dev/null +++ b/contrib/testnet-6000/genesis_examples/proposal_examples/upgrade_proposal.json @@ -0,0 +1,12 @@ +{ + "type": "cosmos-sdk/SoftwareUpgradeProposal", + "value": { + "title": "Upgrade to v0.24.1", + "description": "This proposal will halt the chain after the time below, and restart when 2/3 validators have installed the newer version.", + "plan": { + "name": "Upgrade to v0.24.1", + "time": "2020-05-15T00:00:00Z", + "info": "[some additional information about the upgrade]" + } + } +} \ No newline at end of file diff --git a/x/committee/abci.go b/x/committee/abci.go index d6ec639c..215aaa42 100644 --- a/x/committee/abci.go +++ b/x/committee/abci.go @@ -9,5 +9,8 @@ import ( // BeginBlocker runs at the start of every block. func BeginBlocker(ctx sdk.Context, _ abci.RequestBeginBlock, k Keeper) { + // enact proposals ignoring their expiry time - they could have received enough votes last block before expiring this block + k.EnactPassedProposals(ctx) + k.CloseExpiredProposals(ctx) } diff --git a/x/committee/abci_test.go b/x/committee/abci_test.go index d112bb30..03741069 100644 --- a/x/committee/abci_test.go +++ b/x/committee/abci_test.go @@ -8,10 +8,14 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/x/gov" + "github.com/cosmos/cosmos-sdk/x/params" + "github.com/cosmos/cosmos-sdk/x/upgrade" abci "github.com/tendermint/tendermint/abci/types" "github.com/kava-labs/kava/app" + "github.com/kava-labs/kava/x/cdp" + cdptypes "github.com/kava-labs/kava/x/cdp/types" "github.com/kava-labs/kava/x/committee" ) @@ -32,7 +36,7 @@ func (suite *ModuleTestSuite) SetupTest() { _, suite.addresses = app.GeneratePrivKeyAddressPairs(5) } -func (suite *ModuleTestSuite) TestBeginBlock() { +func (suite *ModuleTestSuite) TestBeginBlock_ClosesExpired() { suite.app.InitializeFromGenesisStates() normalCom := committee.Committee{ @@ -44,12 +48,12 @@ func (suite *ModuleTestSuite) TestBeginBlock() { } suite.keeper.SetCommittee(suite.ctx, normalCom) - pprop1 := gov.NewTextProposal("1A Title", "A description of this proposal.") + pprop1 := gov.NewTextProposal("Title 1", "A description of this proposal.") id1, err := suite.keeper.SubmitProposal(suite.ctx, normalCom.Members[0], normalCom.ID, pprop1) suite.NoError(err) oneHrLaterCtx := suite.ctx.WithBlockTime(suite.ctx.BlockTime().Add(time.Hour)) - pprop2 := gov.NewTextProposal("2A Title", "A description of this proposal.") + pprop2 := gov.NewTextProposal("Title 2", "A description of this proposal.") id2, err := suite.keeper.SubmitProposal(oneHrLaterCtx, normalCom.Members[0], normalCom.ID, pprop2) suite.NoError(err) @@ -66,6 +70,161 @@ func (suite *ModuleTestSuite) TestBeginBlock() { suite.True(found, "expected non expired proposal to be not closed") } +func (suite *ModuleTestSuite) TestBeginBlock_EnactsPassed() { + suite.app.InitializeFromGenesisStates() + + // setup committee + normalCom := committee.Committee{ + ID: 12, + Members: suite.addresses[:2], + Permissions: []committee.Permission{committee.GodPermission{}}, + VoteThreshold: d("0.8"), + ProposalDuration: time.Hour * 24 * 7, + } + suite.keeper.SetCommittee(suite.ctx, normalCom) + + // setup 2 proposals + previousCDPDebtThreshold := suite.app.GetCDPKeeper().GetParams(suite.ctx).DebtAuctionThreshold + newDebtThreshold := previousCDPDebtThreshold.Add(i(1000000)) + evenNewerDebtThreshold := newDebtThreshold.Add(i(1000000)) + + pprop1 := params.NewParameterChangeProposal("Title 1", "A description of this proposal.", + []params.ParamChange{{ + Subspace: cdptypes.ModuleName, + Key: string(cdp.KeyDebtThreshold), + Value: string(cdp.ModuleCdc.MustMarshalJSON(newDebtThreshold)), + }}, + ) + id1, err := suite.keeper.SubmitProposal(suite.ctx, normalCom.Members[0], normalCom.ID, pprop1) + suite.NoError(err) + + pprop2 := params.NewParameterChangeProposal("Title 2", "A description of this proposal.", + []params.ParamChange{{ + Subspace: cdptypes.ModuleName, + Key: string(cdp.KeyDebtThreshold), + Value: string(cdp.ModuleCdc.MustMarshalJSON(evenNewerDebtThreshold)), + }}, + ) + id2, err := suite.keeper.SubmitProposal(suite.ctx, normalCom.Members[0], normalCom.ID, pprop2) + suite.NoError(err) + + // add enough votes to make the first proposal pass, but not the second + suite.NoError(suite.keeper.AddVote(suite.ctx, id1, suite.addresses[0])) + suite.NoError(suite.keeper.AddVote(suite.ctx, id1, suite.addresses[1])) + suite.NoError(suite.keeper.AddVote(suite.ctx, id2, suite.addresses[0])) + + // Run BeginBlocker + suite.NotPanics(func() { + committee.BeginBlocker(suite.ctx, abci.RequestBeginBlock{}, suite.keeper) + }) + + // Check the param has been updated + suite.Equal(newDebtThreshold, suite.app.GetCDPKeeper().GetParams(suite.ctx).DebtAuctionThreshold) + // Check the passed proposal has gone + _, found := suite.keeper.GetProposal(suite.ctx, id1) + suite.False(found, "expected passed proposal to be enacted and closed") + _, found = suite.keeper.GetProposal(suite.ctx, id2) + suite.True(found, "expected non passed proposal to be not closed") +} + +func (suite *ModuleTestSuite) TestBeginBlock_DoesntEnactFailed() { + suite.app.InitializeFromGenesisStates() + + // setup committee + normalCom := committee.Committee{ + ID: 12, + Members: suite.addresses[:1], + Permissions: []committee.Permission{committee.SoftwareUpgradePermission{}}, + VoteThreshold: d("1.0"), + ProposalDuration: time.Hour * 24 * 7, + } + firstBlockTime := time.Date(1998, 1, 1, 0, 0, 0, 0, time.UTC) + ctx := suite.ctx.WithBlockTime(firstBlockTime) + suite.keeper.SetCommittee(ctx, normalCom) + + // setup an upgrade proposal + pprop1 := upgrade.NewSoftwareUpgradeProposal("Title 1", "A description of this proposal.", + upgrade.Plan{ + Name: "upgrade-version-v0.23.1", + Time: firstBlockTime.Add(time.Second * 5), + Info: "some information about the upgrade", + }, + ) + id1, err := suite.keeper.SubmitProposal(ctx, normalCom.Members[0], normalCom.ID, pprop1) + suite.NoError(err) + + // add enough votes to make the proposal pass + suite.NoError(suite.keeper.AddVote(ctx, id1, suite.addresses[0])) + + // Run BeginBlocker 10 seconds later (5 seconds after upgrade expires) + tenSecLaterCtx := ctx.WithBlockTime(ctx.BlockTime().Add(time.Second * 10)) + suite.NotPanics(func() { + suite.app.BeginBlocker(tenSecLaterCtx, abci.RequestBeginBlock{}) + }) + + // Check the plan has not been stored + _, found := suite.app.GetUpgradeKeeper().GetUpgradePlan(tenSecLaterCtx) + suite.False(found) + // Check the passed proposal has gone + _, found = suite.keeper.GetProposal(tenSecLaterCtx, id1) + suite.False(found, "expected failed proposal to be not enacted and closed") + + // Check the chain doesn't halt + oneMinLaterCtx := ctx.WithBlockTime(ctx.BlockTime().Add(time.Minute).Add(time.Second)) + suite.NotPanics(func() { + suite.app.BeginBlocker(oneMinLaterCtx, abci.RequestBeginBlock{}) + }) +} + +func (suite *ModuleTestSuite) TestBeginBlock_EnactsPassedUpgrade() { + suite.app.InitializeFromGenesisStates() + + // setup committee + normalCom := committee.Committee{ + ID: 12, + Members: suite.addresses[:1], + Permissions: []committee.Permission{committee.SoftwareUpgradePermission{}}, + VoteThreshold: d("1.0"), + ProposalDuration: time.Hour * 24 * 7, + } + firstBlockTime := time.Date(1998, 1, 1, 0, 0, 0, 0, time.UTC) + ctx := suite.ctx.WithBlockTime(firstBlockTime) + suite.keeper.SetCommittee(ctx, normalCom) + + // setup an upgrade proposal + pprop1 := upgrade.NewSoftwareUpgradeProposal("Title 1", "A description of this proposal.", + upgrade.Plan{ + Name: "upgrade-version-v0.23.1", + Time: firstBlockTime.Add(time.Minute * 1), + Info: "some information about the upgrade", + }, + ) + id1, err := suite.keeper.SubmitProposal(ctx, normalCom.Members[0], normalCom.ID, pprop1) + suite.NoError(err) + + // add enough votes to make the proposal pass + suite.NoError(suite.keeper.AddVote(ctx, id1, suite.addresses[0])) + + // Run BeginBlocker + fiveSecLaterCtx := ctx.WithBlockTime(ctx.BlockTime().Add(time.Second * 5)) + suite.NotPanics(func() { + suite.app.BeginBlocker(fiveSecLaterCtx, abci.RequestBeginBlock{}) + }) + + // Check the plan has been stored + _, found := suite.app.GetUpgradeKeeper().GetUpgradePlan(fiveSecLaterCtx) + suite.True(found) + // Check the passed proposal has gone + _, found = suite.keeper.GetProposal(fiveSecLaterCtx, id1) + suite.False(found, "expected passed proposal to be enacted and closed") + + // Check the chain halts + oneMinLaterCtx := ctx.WithBlockTime(ctx.BlockTime().Add(time.Minute)) + suite.Panics(func() { + suite.app.BeginBlocker(oneMinLaterCtx, abci.RequestBeginBlock{}) + }) +} + func TestModuleTestSuite(t *testing.T) { suite.Run(t, new(ModuleTestSuite)) } diff --git a/x/committee/alias.go b/x/committee/alias.go index 524448dc..e9f2fa72 100644 --- a/x/committee/alias.go +++ b/x/committee/alias.go @@ -61,6 +61,7 @@ var ( NewQueryCommitteeParams = types.NewQueryCommitteeParams NewQueryProposalParams = types.NewQueryProposalParams NewQueryVoteParams = types.NewQueryVoteParams + NewVote = types.NewVote RegisterCodec = types.RegisterCodec RegisterPermissionTypeCodec = types.RegisterPermissionTypeCodec RegisterProposalTypeCodec = types.RegisterProposalTypeCodec @@ -84,23 +85,33 @@ var ( ) type ( - Keeper = keeper.Keeper - AllowedParam = types.AllowedParam - AllowedParams = types.AllowedParams - Committee = types.Committee - CommitteeChangeProposal = types.CommitteeChangeProposal - CommitteeDeleteProposal = types.CommitteeDeleteProposal - GenesisState = types.GenesisState - GodPermission = types.GodPermission - MsgSubmitProposal = types.MsgSubmitProposal - MsgVote = types.MsgVote - ParamChangePermission = types.ParamChangePermission - Permission = types.Permission - Proposal = types.Proposal - PubProposal = types.PubProposal - QueryCommitteeParams = types.QueryCommitteeParams - QueryProposalParams = types.QueryProposalParams - QueryVoteParams = types.QueryVoteParams - TextPermission = types.TextPermission - Vote = types.Vote + Keeper = keeper.Keeper + AllowedAssetParam = types.AllowedAssetParam + AllowedAssetParams = types.AllowedAssetParams + AllowedCollateralParam = types.AllowedCollateralParam + AllowedCollateralParams = types.AllowedCollateralParams + AllowedDebtParam = types.AllowedDebtParam + AllowedMarket = types.AllowedMarket + AllowedMarkets = types.AllowedMarkets + AllowedParam = types.AllowedParam + AllowedParams = types.AllowedParams + Committee = types.Committee + CommitteeChangeProposal = types.CommitteeChangeProposal + CommitteeDeleteProposal = types.CommitteeDeleteProposal + GenesisState = types.GenesisState + GodPermission = types.GodPermission + MsgSubmitProposal = types.MsgSubmitProposal + MsgVote = types.MsgVote + ParamKeeper = types.ParamKeeper + Permission = types.Permission + Proposal = types.Proposal + PubProposal = types.PubProposal + QueryCommitteeParams = types.QueryCommitteeParams + QueryProposalParams = types.QueryProposalParams + QueryVoteParams = types.QueryVoteParams + SimpleParamChangePermission = types.SimpleParamChangePermission + SoftwareUpgradePermission = types.SoftwareUpgradePermission + SubParamChangePermission = types.SubParamChangePermission + TextPermission = types.TextPermission + Vote = types.Vote ) diff --git a/x/committee/client/cli/tx.go b/x/committee/client/cli/tx.go index d4575a47..61dda2bb 100644 --- a/x/committee/client/cli/tx.go +++ b/x/committee/client/cli/tx.go @@ -197,7 +197,7 @@ func MustGetExampleCommitteeChangeProposal(cdc *codec.Codec) string { "The description of this committee.", []sdk.AccAddress{sdk.AccAddress(crypto.AddressHash([]byte("exampleAddress")))}, []types.Permission{ - types.ParamChangePermission{ + types.SimpleParamChangePermission{ AllowedParams: types.AllowedParams{{Subspace: "cdp", Key: "CircuitBreaker"}}, }, }, diff --git a/x/committee/handler.go b/x/committee/handler.go index e46c0b6a..011c5040 100644 --- a/x/committee/handler.go +++ b/x/committee/handler.go @@ -1,8 +1,6 @@ package committee import ( - "fmt" - sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" @@ -47,16 +45,11 @@ func handleMsgSubmitProposal(ctx sdk.Context, k keeper.Keeper, msg types.MsgSubm } func handleMsgVote(ctx sdk.Context, k keeper.Keeper, msg types.MsgVote) (*sdk.Result, error) { - // get the proposal just to add fields to the event - proposal, found := k.GetProposal(ctx, msg.ProposalID) - if !found { - return nil, sdkerrors.Wrapf(ErrUnknownProposal, "%d", msg.ProposalID) - } - err := k.AddVote(ctx, msg.ProposalID, msg.Voter) if err != nil { return nil, err } + ctx.EventManager().EmitEvent( sdk.NewEvent( sdk.EventTypeMessage, @@ -65,31 +58,5 @@ func handleMsgVote(ctx sdk.Context, k keeper.Keeper, msg types.MsgVote) (*sdk.Re ), ) - // Enact a proposal if it has enough votes - passes, err := k.GetProposalResult(ctx, msg.ProposalID) - if err != nil { - return nil, err - } - if !passes { - return &sdk.Result{Events: ctx.EventManager().Events()}, nil - } - - err = k.EnactProposal(ctx, msg.ProposalID) - outcome := types.AttributeValueProposalPassed - if err != nil { - outcome = types.AttributeValueProposalFailed - } - - k.DeleteProposalAndVotes(ctx, msg.ProposalID) - - ctx.EventManager().EmitEvent( - sdk.NewEvent( - types.EventTypeProposalClose, - sdk.NewAttribute(types.AttributeKeyCommitteeID, fmt.Sprintf("%d", proposal.CommitteeID)), - sdk.NewAttribute(types.AttributeKeyProposalID, fmt.Sprintf("%d", proposal.ID)), - sdk.NewAttribute(types.AttributeKeyProposalCloseStatus, outcome), - ), - ) - return &sdk.Result{Events: ctx.EventManager().Events()}, nil } diff --git a/x/committee/handler_test.go b/x/committee/handler_test.go index 2b71239e..841121ee 100644 --- a/x/committee/handler_test.go +++ b/x/committee/handler_test.go @@ -9,6 +9,7 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/x/distribution" "github.com/cosmos/cosmos-sdk/x/params" + "github.com/cosmos/cosmos-sdk/x/upgrade" abci "github.com/tendermint/tendermint/abci/types" @@ -116,6 +117,28 @@ func (suite *HandlerTestSuite) TestSubmitProposalMsg_Invalid() { } +func (suite *HandlerTestSuite) TestSubmitProposalMsg_ValidUpgrade() { + msg := committee.NewMsgSubmitProposal( + upgrade.NewSoftwareUpgradeProposal( + "A Title", + "A description of this proposal.", + upgrade.Plan{ + Name: "emergency-shutdown-1", // identifier for the upgrade + Time: suite.ctx.BlockTime().Add(time.Minute * 10), // time after which to implement plan + Info: "Some information about the shutdown.", + }, + ), + suite.addresses[0], + 1, + ) + + res, err := suite.handler(suite.ctx, msg) + + suite.NoError(err) + _, found := suite.keeper.GetProposal(suite.ctx, types.Uint64FromBytes(res.Data)) + suite.True(found) +} + func (suite *HandlerTestSuite) TestSubmitProposalMsg_Unregistered() { var committeeID uint64 = 1 msg := types.NewMsgSubmitProposal( @@ -133,80 +156,6 @@ func (suite *HandlerTestSuite) TestSubmitProposalMsg_Unregistered() { ) } -func (suite *HandlerTestSuite) TestMsgAddVote_ProposalPass() { - previousCDPDebtThreshold := suite.app.GetCDPKeeper().GetParams(suite.ctx).DebtAuctionThreshold - newDebtThreshold := previousCDPDebtThreshold.Add(i(1000000)) - msg := types.NewMsgSubmitProposal( - params.NewParameterChangeProposal( - "A Title", - "A description of this proposal.", - []params.ParamChange{{ - Subspace: cdptypes.ModuleName, - Key: string(cdptypes.KeyDebtThreshold), - Value: string(types.ModuleCdc.MustMarshalJSON(newDebtThreshold)), - }}, - ), - suite.addresses[0], - 1, - ) - res, err := suite.handler(suite.ctx, msg) - suite.NoError(err) - proposalID := types.Uint64FromBytes(res.Data) - _, err = suite.handler(suite.ctx, types.NewMsgVote(suite.addresses[0], proposalID)) - suite.NoError(err) - - // Add a vote to make the proposal pass - _, err = suite.handler(suite.ctx, types.NewMsgVote(suite.addresses[1], proposalID)) - - suite.NoError(err) - // Check the param has been updated - suite.Equal(newDebtThreshold, suite.app.GetCDPKeeper().GetParams(suite.ctx).DebtAuctionThreshold) - // Check proposal and votes are gone - _, found := suite.keeper.GetProposal(suite.ctx, proposalID) - suite.False(found) - suite.Empty( - suite.keeper.GetVotesByProposal(suite.ctx, proposalID), - "vote found when there should be none", - ) -} - -func (suite *HandlerTestSuite) TestMsgAddVote_ProposalFail() { - recipient := suite.addresses[4] - recipientCoins := suite.app.GetBankKeeper().GetCoins(suite.ctx, recipient) - msg := types.NewMsgSubmitProposal( - distribution.NewCommunityPoolSpendProposal( - "A Title", - "A description of this proposal.", - recipient, - cs(c("ukava", 500)), - ), - suite.addresses[0], - 1, - ) - res, err := suite.handler(suite.ctx, msg) - suite.NoError(err) - proposalID := types.Uint64FromBytes(res.Data) - _, err = suite.handler(suite.ctx, types.NewMsgVote(suite.addresses[0], proposalID)) - suite.NoError(err) - - // invalidate the proposal by emptying community pool - suite.app.GetDistrKeeper().DistributeFromFeePool(suite.ctx, suite.communityPoolAmt, suite.addresses[0]) - - // Add a vote to make the proposal pass - _, err = suite.handler(suite.ctx, types.NewMsgVote(suite.addresses[1], proposalID)) - - suite.NoError(err) - // Check the proposal was not enacted - suite.Equal(recipientCoins, suite.app.GetBankKeeper().GetCoins(suite.ctx, recipient)) - // Check proposal and votes are gone - _, found := suite.keeper.GetProposal(suite.ctx, proposalID) - suite.False(found) - suite.Empty( - suite.keeper.GetVotesByProposal(suite.ctx, proposalID), - "vote found when there should be none", - ) -} - func TestHandlerTestSuite(t *testing.T) { suite.Run(t, new(HandlerTestSuite)) } diff --git a/x/committee/types/committee_test.go b/x/committee/keeper/committee_test.go similarity index 73% rename from x/committee/types/committee_test.go rename to x/committee/keeper/committee_test.go index 18df9f30..5dcc78d0 100644 --- a/x/committee/types/committee_test.go +++ b/x/committee/keeper/committee_test.go @@ -1,24 +1,19 @@ -package types +package keeper_test import ( "testing" "time" "github.com/stretchr/testify/suite" + abci "github.com/tendermint/tendermint/abci/types" govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" paramstypes "github.com/cosmos/cosmos-sdk/x/params/types" + + "github.com/kava-labs/kava/app" + "github.com/kava-labs/kava/x/committee/types" ) -var _ PubProposal = UnregisteredPubProposal{} - -type UnregisteredPubProposal struct { - govtypes.TextProposal -} - -func (UnregisteredPubProposal) ProposalRoute() string { return "unregistered" } -func (UnregisteredPubProposal) ProposalType() string { return "unregistered" } - type TypesTestSuite struct { suite.Suite } @@ -27,14 +22,14 @@ func (suite *TypesTestSuite) TestCommittee_HasPermissionsFor() { testcases := []struct { name string - permissions []Permission - pubProposal PubProposal + permissions []types.Permission + pubProposal types.PubProposal expectHasPermissions bool }{ { name: "normal (single permission)", - permissions: []Permission{ParamChangePermission{ - AllowedParams: AllowedParams{ + permissions: []types.Permission{types.SimpleParamChangePermission{ + AllowedParams: types.AllowedParams{ { Subspace: "cdp", Key: "DebtThreshold", @@ -56,30 +51,30 @@ func (suite *TypesTestSuite) TestCommittee_HasPermissionsFor() { }, { name: "normal (multiple permissions)", - permissions: []Permission{ - ParamChangePermission{ - AllowedParams: AllowedParams{ + permissions: []types.Permission{ + types.SimpleParamChangePermission{ + AllowedParams: types.AllowedParams{ { Subspace: "cdp", Key: "DebtThreshold", }, }}, - TextPermission{}, + types.TextPermission{}, }, pubProposal: govtypes.NewTextProposal("A Proposal Title", "A description of this proposal"), expectHasPermissions: true, }, { name: "overruling permission", - permissions: []Permission{ - ParamChangePermission{ - AllowedParams: AllowedParams{ + permissions: []types.Permission{ + types.SimpleParamChangePermission{ + AllowedParams: types.AllowedParams{ { Subspace: "cdp", Key: "DebtThreshold", }, }}, - GodPermission{}, + types.GodPermission{}, }, pubProposal: paramstypes.NewParameterChangeProposal( "A Title", @@ -115,16 +110,16 @@ func (suite *TypesTestSuite) TestCommittee_HasPermissionsFor() { { name: "split permissions", // These permissions looks like they allow the param change proposal, however a proposal must pass a single permission independently of others. - permissions: []Permission{ - ParamChangePermission{ - AllowedParams: AllowedParams{ + permissions: []types.Permission{ + types.SimpleParamChangePermission{ + AllowedParams: types.AllowedParams{ { Subspace: "cdp", Key: "DebtThreshold", }, }}, - ParamChangePermission{ - AllowedParams: AllowedParams{ + types.SimpleParamChangePermission{ + AllowedParams: types.AllowedParams{ { Subspace: "cdp", Key: "DebtParams", @@ -153,9 +148,9 @@ func (suite *TypesTestSuite) TestCommittee_HasPermissionsFor() { }, { name: "unregistered proposal", - permissions: []Permission{ - ParamChangePermission{ - AllowedParams: AllowedParams{ + permissions: []types.Permission{ + types.SimpleParamChangePermission{ + AllowedParams: types.AllowedParams{ { Subspace: "cdp", Key: "DebtThreshold", @@ -169,7 +164,10 @@ func (suite *TypesTestSuite) TestCommittee_HasPermissionsFor() { for _, tc := range testcases { suite.Run(tc.name, func() { - com := NewCommittee( + tApp := app.NewTestApp() + ctx := tApp.NewContext(true, abci.Header{}) + tApp.InitializeFromGenesisStates() + com := types.NewCommittee( 12, "a description of this committee", nil, @@ -179,7 +177,7 @@ func (suite *TypesTestSuite) TestCommittee_HasPermissionsFor() { ) suite.Equal( tc.expectHasPermissions, - com.HasPermissionsFor(tc.pubProposal), + com.HasPermissionsFor(ctx, tApp.Codec(), tApp.GetParamsKeeper(), tc.pubProposal), ) }) } diff --git a/x/committee/keeper/invariants.go b/x/committee/keeper/invariants.go index d5202ae2..f8ff6979 100644 --- a/x/committee/keeper/invariants.go +++ b/x/committee/keeper/invariants.go @@ -71,17 +71,12 @@ func ValidProposalsInvariant(k Keeper) sdk.Invariant { } } - com, found := k.GetCommittee(ctx, proposal.CommitteeID) + _, found := k.GetCommittee(ctx, proposal.CommitteeID) if !found { validationErr = fmt.Errorf("proposal has no committee %d", proposal.CommitteeID) return true } - if !com.HasPermissionsFor(proposal.PubProposal) { - validationErr = fmt.Errorf("proposal not permitted for committee %+v", com) - return true - } - return false }) @@ -107,8 +102,8 @@ func ValidVotesInvariant(k Keeper) sdk.Invariant { k.IterateVotes(ctx, func(vote types.Vote) bool { invalidVote = vote - if vote.Voter.Empty() { - validationErr = fmt.Errorf("empty voter address") + if err := vote.Validate(); err != nil { + validationErr = err return true } diff --git a/x/committee/keeper/keeper.go b/x/committee/keeper/keeper.go index e6b3d9c8..4e9de243 100644 --- a/x/committee/keeper/keeper.go +++ b/x/committee/keeper/keeper.go @@ -16,19 +16,22 @@ type Keeper struct { cdc *codec.Codec storeKey sdk.StoreKey + ParamKeeper types.ParamKeeper // TODO ideally don't export, only sims need it exported + // Proposal router router govtypes.Router } -func NewKeeper(cdc *codec.Codec, storeKey sdk.StoreKey, router govtypes.Router) Keeper { +func NewKeeper(cdc *codec.Codec, storeKey sdk.StoreKey, router govtypes.Router, paramKeeper types.ParamKeeper) Keeper { // Logic in the keeper methods assume the set of gov handlers is fixed. // So the gov router must be sealed so no handlers can be added or removed after the keeper is created. router.Seal() return Keeper{ - cdc: cdc, - storeKey: storeKey, - router: router, + cdc: cdc, + storeKey: storeKey, + ParamKeeper: paramKeeper, + router: router, } } @@ -271,11 +274,14 @@ func (k Keeper) GetVotes(ctx sdk.Context) []types.Vote { // GetVotesByProposal returns all votes for one proposal. func (k Keeper) GetVotesByProposal(ctx sdk.Context, proposalID uint64) []types.Vote { results := []types.Vote{} - k.IterateVotes(ctx, func(vote types.Vote) bool { - if vote.ProposalID == proposalID { - results = append(results, vote) - } - return false - }) + iterator := sdk.KVStorePrefixIterator(ctx.KVStore(k.storeKey), append(types.VoteKeyPrefix, types.GetKeyFromID(proposalID)...)) + + defer iterator.Close() + for ; iterator.Valid(); iterator.Next() { + var vote types.Vote + k.cdc.MustUnmarshalBinaryBare(iterator.Value(), &vote) + results = append(results, vote) + } + return results } diff --git a/x/committee/keeper/param_permission_test.go b/x/committee/keeper/param_permission_test.go new file mode 100644 index 00000000..726b2727 --- /dev/null +++ b/x/committee/keeper/param_permission_test.go @@ -0,0 +1,239 @@ +package keeper_test + +import ( + "testing" + + "github.com/cosmos/cosmos-sdk/codec" + sdk "github.com/cosmos/cosmos-sdk/types" + govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" + paramstypes "github.com/cosmos/cosmos-sdk/x/params/types" + "github.com/stretchr/testify/suite" + abci "github.com/tendermint/tendermint/abci/types" + + "github.com/kava-labs/kava/app" + bep3types "github.com/kava-labs/kava/x/bep3/types" + cdptypes "github.com/kava-labs/kava/x/cdp/types" + "github.com/kava-labs/kava/x/committee/types" + pricefeedtypes "github.com/kava-labs/kava/x/pricefeed/types" +) + +type PermissionTestSuite struct { + suite.Suite + cdc *codec.Codec +} + +func (suite *PermissionTestSuite) SetupTest() { + app := app.NewTestApp() + suite.cdc = app.Codec() +} + +func (suite *PermissionTestSuite) TestSubParamChangePermission_Allows() { + // cdp CollateralParams + testCPs := cdptypes.CollateralParams{ + { + Denom: "bnb", + LiquidationRatio: d("2.0"), + DebtLimit: c("usdx", 1000000000000), + StabilityFee: d("1.000000001547125958"), + LiquidationPenalty: d("0.05"), + AuctionSize: i(100), + Prefix: 0x20, + ConversionFactor: i(6), + MarketID: "bnb:usd", + }, + { + Denom: "btc", + LiquidationRatio: d("1.5"), + DebtLimit: c("usdx", 1000000000), + StabilityFee: d("1.000000001547125958"), + LiquidationPenalty: d("0.10"), + AuctionSize: i(1000), + Prefix: 0x30, + ConversionFactor: i(8), + MarketID: "btc:usd", + }, + } + testCPUpdatedDebtLimit := make(cdptypes.CollateralParams, len(testCPs)) + copy(testCPUpdatedDebtLimit, testCPs) + testCPUpdatedDebtLimit[0].DebtLimit = c("usdx", 5000000) + + // cdp DebtParam + testDP := cdptypes.DebtParam{ + Denom: "usdx", + ReferenceAsset: "usd", + ConversionFactor: i(6), + DebtFloor: i(10000000), + SavingsRate: d("0.95"), + } + testDPUpdatedDebtFloor := testDP + testDPUpdatedDebtFloor.DebtFloor = i(1000) + + // cdp Genesis + testCDPParams := cdptypes.DefaultParams() + testCDPParams.CollateralParams = testCPs + testCDPParams.DebtParam = testDP + testCDPParams.GlobalDebtLimit = testCPs[0].DebtLimit.Add(testCPs[0].DebtLimit) // correct global debt limit to pass genesis validation + + // bep3 Asset Params + testAPs := bep3types.AssetParams{ + { + Denom: "bnb", + CoinID: 714, + Limit: i(100000000000), + Active: true, + }, + { + Denom: "inc", + CoinID: 9999, + Limit: i(100), + Active: false, + }, + } + testAPsUpdatedActive := make(bep3types.AssetParams, len(testAPs)) + copy(testAPsUpdatedActive, testAPs) + testAPsUpdatedActive[1].Active = true + + // bep3 Genesis + testBep3Params := bep3types.DefaultParams() + testBep3Params.SupportedAssets = testAPs + + // pricefeed Markets + testMs := pricefeedtypes.Markets{ + { + MarketID: "bnb:usd", + BaseAsset: "bnb", + QuoteAsset: "usd", + Oracles: []sdk.AccAddress{}, + Active: true, + }, + { + MarketID: "btc:usd", + BaseAsset: "btc", + QuoteAsset: "usd", + Oracles: []sdk.AccAddress{}, + Active: true, + }, + } + testMsUpdatedActive := make(pricefeedtypes.Markets, len(testMs)) + copy(testMsUpdatedActive, testMs) + testMsUpdatedActive[1].Active = true + + testcases := []struct { + name string + genState []app.GenesisState + permission types.SubParamChangePermission + pubProposal types.PubProposal + expectAllowed bool + }{ + { + name: "normal", + genState: []app.GenesisState{ + newPricefeedGenState([]string{"bnb", "btc"}, []sdk.Dec{d("15.01"), d("9500")}), + newCDPGenesisState(testCDPParams), + newBep3GenesisState(testBep3Params), + }, + permission: types.SubParamChangePermission{ + AllowedParams: types.AllowedParams{ + {Subspace: cdptypes.ModuleName, Key: string(cdptypes.KeyDebtThreshold)}, + {Subspace: cdptypes.ModuleName, Key: string(cdptypes.KeyCollateralParams)}, + {Subspace: cdptypes.ModuleName, Key: string(cdptypes.KeyDebtParam)}, + {Subspace: bep3types.ModuleName, Key: string(bep3types.KeySupportedAssets)}, + {Subspace: pricefeedtypes.ModuleName, Key: string(pricefeedtypes.KeyMarkets)}, + }, + AllowedCollateralParams: types.AllowedCollateralParams{ + { + Denom: "bnb", + DebtLimit: true, + StabilityFee: true, + }, + { // TODO currently even if a perm doesn't allow a change in one element it must still be present in list + Denom: "btc", + }, + }, + AllowedDebtParam: types.AllowedDebtParam{ + DebtFloor: true, + }, + AllowedAssetParams: types.AllowedAssetParams{ + { + Denom: "bnb", + }, + { + Denom: "inc", + Active: true, + }, + }, + AllowedMarkets: types.AllowedMarkets{ + { + MarketID: "bnb:usd", + }, + { + MarketID: "btc:usd", + Active: true, + }, + }, + }, + pubProposal: paramstypes.NewParameterChangeProposal( + "A Title", + "A description for this proposal.", + []paramstypes.ParamChange{ + { + Subspace: cdptypes.ModuleName, + Key: string(cdptypes.KeyDebtThreshold), + Value: string(suite.cdc.MustMarshalJSON(i(1234))), + }, + { + Subspace: cdptypes.ModuleName, + Key: string(cdptypes.KeyCollateralParams), + Value: string(suite.cdc.MustMarshalJSON(testCPUpdatedDebtLimit)), + }, + { + Subspace: cdptypes.ModuleName, + Key: string(cdptypes.KeyDebtParam), + Value: string(suite.cdc.MustMarshalJSON(testDPUpdatedDebtFloor)), + }, + { + Subspace: bep3types.ModuleName, + Key: string(bep3types.KeySupportedAssets), + Value: string(suite.cdc.MustMarshalJSON(testAPsUpdatedActive)), + }, + { + Subspace: pricefeedtypes.ModuleName, + Key: string(pricefeedtypes.KeyMarkets), + Value: string(suite.cdc.MustMarshalJSON(testMsUpdatedActive)), + }, + }, + ), + expectAllowed: true, + }, + { + name: "not allowed (wrong pubproposal type)", + permission: types.SubParamChangePermission{}, + pubProposal: govtypes.NewTextProposal("A Title", "A description for this proposal."), + expectAllowed: false, + }, + { + name: "not allowed (nil pubproposal)", + permission: types.SubParamChangePermission{}, + pubProposal: nil, + expectAllowed: false, + }, + // TODO more cases + } + + for _, tc := range testcases { + suite.Run(tc.name, func() { + tApp := app.NewTestApp() + ctx := tApp.NewContext(true, abci.Header{}) + tApp.InitializeFromGenesisStates(tc.genState...) + + suite.Equal( + tc.expectAllowed, + tc.permission.Allows(ctx, tApp.Codec(), tApp.GetParamsKeeper(), tc.pubProposal), + ) + }) + } + +} +func TestPermissionTestSuite(t *testing.T) { + suite.Run(t, new(PermissionTestSuite)) +} diff --git a/x/committee/keeper/proposal.go b/x/committee/keeper/proposal.go index 46ac5b9e..017ef589 100644 --- a/x/committee/keeper/proposal.go +++ b/x/committee/keeper/proposal.go @@ -21,7 +21,7 @@ func (k Keeper) SubmitProposal(ctx sdk.Context, proposer sdk.AccAddress, committ } // Check committee has permissions to enact proposal. - if !com.HasPermissionsFor(pubProposal) { + if !com.HasPermissionsFor(ctx, k.cdc, k.ParamKeeper, pubProposal) { return 0, sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "committee does not have permissions to enact proposal") } @@ -67,7 +67,7 @@ func (k Keeper) AddVote(ctx sdk.Context, proposalID uint64, voter sdk.AccAddress } // Store vote, overwriting any prior vote - k.SetVote(ctx, types.Vote{ProposalID: proposalID, Voter: voter}) + k.SetVote(ctx, types.NewVote(proposalID, voter)) ctx.EventManager().EmitEvent( sdk.NewEvent( @@ -107,23 +107,61 @@ func (k Keeper) TallyVotes(ctx sdk.Context, proposalID uint64) int64 { } // EnactProposal makes the changes proposed in a proposal. -func (k Keeper) EnactProposal(ctx sdk.Context, proposalID uint64) error { - pr, found := k.GetProposal(ctx, proposalID) +func (k Keeper) EnactProposal(ctx sdk.Context, proposal types.Proposal) error { + // Check committee still has permissions for the proposal + // Since the proposal was submitted params could have changed, invalidating the permission of the committee. + com, found := k.GetCommittee(ctx, proposal.CommitteeID) if !found { - return sdkerrors.Wrapf(types.ErrUnknownProposal, "%d", proposalID) + return sdkerrors.Wrapf(types.ErrUnknownCommittee, "%d", proposal.CommitteeID) + } + if !com.HasPermissionsFor(ctx, k.cdc, k.ParamKeeper, proposal.PubProposal) { + return sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "committee does not have permissions to enact proposal") } - if err := k.ValidatePubProposal(ctx, pr.PubProposal); err != nil { + if err := k.ValidatePubProposal(ctx, proposal.PubProposal); err != nil { return err } - handler := k.router.GetRoute(pr.ProposalRoute()) - if err := handler(ctx, pr.PubProposal); err != nil { + + // enact the proposal + handler := k.router.GetRoute(proposal.ProposalRoute()) + if err := handler(ctx, proposal.PubProposal); err != nil { // the handler should not error as it was checked in ValidatePubProposal panic(fmt.Sprintf("unexpected handler error: %s", err)) } return nil } +// EnactPassedProposals puts in place the changes proposed in any proposal that has enough votes +func (k Keeper) EnactPassedProposals(ctx sdk.Context) { + k.IterateProposals(ctx, func(proposal types.Proposal) bool { + passes, err := k.GetProposalResult(ctx, proposal.ID) + if err != nil { + panic(err) + } + if !passes { + return false + } + + err = k.EnactProposal(ctx, proposal) + outcome := types.AttributeValueProposalPassed + if err != nil { + outcome = types.AttributeValueProposalFailed + } + + k.DeleteProposalAndVotes(ctx, proposal.ID) + + ctx.EventManager().EmitEvent( + sdk.NewEvent( + types.EventTypeProposalClose, + sdk.NewAttribute(types.AttributeKeyCommitteeID, fmt.Sprintf("%d", proposal.CommitteeID)), + sdk.NewAttribute(types.AttributeKeyProposalID, fmt.Sprintf("%d", proposal.ID)), + sdk.NewAttribute(types.AttributeKeyProposalCloseStatus, outcome), + ), + ) + return false + }) +} + // CloseExpiredProposals removes proposals (and associated votes) that have past their deadline. func (k Keeper) CloseExpiredProposals(ctx sdk.Context) { diff --git a/x/committee/keeper/proposal_test.go b/x/committee/keeper/proposal_test.go index 5d0e46e7..9a3d5b67 100644 --- a/x/committee/keeper/proposal_test.go +++ b/x/committee/keeper/proposal_test.go @@ -12,11 +12,49 @@ import ( abci "github.com/tendermint/tendermint/abci/types" "github.com/kava-labs/kava/app" + bep3types "github.com/kava-labs/kava/x/bep3/types" cdptypes "github.com/kava-labs/kava/x/cdp/types" "github.com/kava-labs/kava/x/committee" "github.com/kava-labs/kava/x/committee/types" + "github.com/kava-labs/kava/x/pricefeed" ) +func newCDPGenesisState(params cdptypes.Params) app.GenesisState { + genesis := cdptypes.DefaultGenesisState() + genesis.Params = params + return app.GenesisState{cdptypes.ModuleName: cdptypes.ModuleCdc.MustMarshalJSON(genesis)} +} + +func newBep3GenesisState(params bep3types.Params) app.GenesisState { + genesis := bep3types.DefaultGenesisState() + genesis.Params = params + return app.GenesisState{bep3types.ModuleName: bep3types.ModuleCdc.MustMarshalJSON(genesis)} +} + +func newPricefeedGenState(assets []string, prices []sdk.Dec) app.GenesisState { + if len(assets) != len(prices) { + panic("assets and prices must be the same length") + } + pfGenesis := pricefeed.DefaultGenesisState() + + for i := range assets { + pfGenesis.Params.Markets = append( + pfGenesis.Params.Markets, + pricefeed.Market{ + MarketID: assets[i] + ":usd", BaseAsset: assets[i], QuoteAsset: "usd", Oracles: []sdk.AccAddress{}, Active: true, + }) + pfGenesis.PostedPrices = append( + pfGenesis.PostedPrices, + pricefeed.PostedPrice{ + MarketID: assets[i] + ":usd", + OracleAddress: sdk.AccAddress{}, + Price: prices[i], + Expiry: time.Date(1998, 1, 1, 0, 0, 0, 0, time.UTC), + }) + } + return app.GenesisState{pricefeed.ModuleName: pricefeed.ModuleCdc.MustMarshalJSON(pfGenesis)} +} + func (suite *KeeperTestSuite) TestSubmitProposal() { normalCom := types.Committee{ ID: 12, @@ -26,9 +64,50 @@ func (suite *KeeperTestSuite) TestSubmitProposal() { VoteThreshold: d("0.667"), ProposalDuration: time.Hour * 24 * 7, } + noPermissionsCom := normalCom noPermissionsCom.Permissions = []types.Permission{} + paramChangePermissionsCom := normalCom + paramChangePermissionsCom.Permissions = []types.Permission{ + types.SubParamChangePermission{ + AllowedParams: types.AllowedParams{ + {Subspace: cdptypes.ModuleName, Key: string(cdptypes.KeyDebtThreshold)}, + {Subspace: cdptypes.ModuleName, Key: string(cdptypes.KeyCollateralParams)}, + }, + AllowedCollateralParams: types.AllowedCollateralParams{ + types.AllowedCollateralParam{ + Denom: "bnb", + DebtLimit: true, + StabilityFee: true, + }, + }, + }, + } + + testCP := cdptypes.CollateralParams{{ + Denom: "bnb", + LiquidationRatio: d("1.5"), + DebtLimit: c("usdx", 1000000000000), + StabilityFee: d("1.000000001547125958"), // %5 apr + LiquidationPenalty: d("0.05"), + AuctionSize: i(100), + Prefix: 0x20, + ConversionFactor: i(6), + MarketID: "bnb:usd", + }} + testCDPParams := cdptypes.DefaultParams() + testCDPParams.CollateralParams = testCP + testCDPParams.GlobalDebtLimit = testCP[0].DebtLimit + + newValidCP := make(cdptypes.CollateralParams, len(testCP)) + copy(newValidCP, testCP) + newValidCP[0].DebtLimit = c("usdx", 500000000000) + + newInvalidCP := make(cdptypes.CollateralParams, len(testCP)) + copy(newInvalidCP, testCP) + newInvalidCP[0].MarketID = "btc:usd" + testcases := []struct { name string committee types.Committee @@ -38,13 +117,28 @@ func (suite *KeeperTestSuite) TestSubmitProposal() { expectErr bool }{ { - name: "normal", + name: "normal text proposal", committee: normalCom, pubProposal: gov.NewTextProposal("A Title", "A description of this proposal."), proposer: normalCom.Members[0], committeeID: normalCom.ID, expectErr: false, }, + { + name: "normal param change proposal", + committee: normalCom, + pubProposal: params.NewParameterChangeProposal( + "A Title", "A description of this proposal.", + []params.ParamChange{ + { + Subspace: "cdp", Key: string(cdptypes.KeyDebtThreshold), Value: string(suite.app.Codec().MustMarshalJSON(i(1000000))), + }, + }, + ), + proposer: normalCom.Members[0], + committeeID: normalCom.ID, + expectErr: false, + }, { name: "invalid proposal", committee: normalCom, @@ -77,6 +171,42 @@ func (suite *KeeperTestSuite) TestSubmitProposal() { committeeID: noPermissionsCom.ID, expectErr: true, }, + { + name: "valid sub param change", + committee: paramChangePermissionsCom, + pubProposal: params.NewParameterChangeProposal( + "A Title", "A description of this proposal.", + []params.ParamChange{ + { + "cdp", string(cdptypes.KeyDebtThreshold), string(suite.app.Codec().MustMarshalJSON(i(1000000000))), + }, + { + "cdp", string(cdptypes.KeyCollateralParams), string(suite.app.Codec().MustMarshalJSON(newValidCP)), + }, + }, + ), + proposer: paramChangePermissionsCom.Members[0], + committeeID: paramChangePermissionsCom.ID, + expectErr: false, + }, + { + name: "invalid sub param change permission", + committee: paramChangePermissionsCom, + pubProposal: params.NewParameterChangeProposal( + "A Title", "A description of this proposal.", + []params.ParamChange{ + { + "cdp", string(cdptypes.KeyDebtThreshold), string(suite.app.Codec().MustMarshalJSON(i(1000000000))), + }, + { + "cdp", string(cdptypes.KeyCollateralParams), string(suite.app.Codec().MustMarshalJSON(newInvalidCP)), + }, + }, + ), + proposer: paramChangePermissionsCom.Members[0], + committeeID: paramChangePermissionsCom.ID, + expectErr: true, + }, } for _, tc := range testcases { @@ -85,7 +215,10 @@ func (suite *KeeperTestSuite) TestSubmitProposal() { tApp := app.NewTestApp() keeper := tApp.GetCommitteeKeeper() ctx := tApp.NewContext(true, abci.Header{}) - tApp.InitializeFromGenesisStates() + tApp.InitializeFromGenesisStates( + newPricefeedGenState([]string{"bnb"}, []sdk.Dec{d("15.01")}), + newCDPGenesisState(testCDPParams), + ) // setup committee (if required) if !(reflect.DeepEqual(tc.committee, types.Committee{})) { keeper.SetCommittee(ctx, tc.committee) diff --git a/x/committee/proposal_handler.go b/x/committee/proposal_handler.go index bae3d4ff..e3043feb 100644 --- a/x/committee/proposal_handler.go +++ b/x/committee/proposal_handler.go @@ -26,13 +26,10 @@ func handleCommitteeChangeProposal(ctx sdk.Context, k Keeper, committeeProposal } // Remove all committee's ongoing proposals - k.IterateProposals(ctx, func(p Proposal) bool { - if p.CommitteeID != committeeProposal.NewCommittee.ID { - return false - } + proposals := k.GetProposalsByCommittee(ctx, committeeProposal.NewCommittee.ID) + for _, p := range proposals { k.DeleteProposalAndVotes(ctx, p.ID) - return false - }) + } // update/create the committee k.SetCommittee(ctx, committeeProposal.NewCommittee) @@ -45,13 +42,10 @@ func handleCommitteeDeleteProposal(ctx sdk.Context, k Keeper, committeeProposal } // Remove all committee's ongoing proposals - k.IterateProposals(ctx, func(p Proposal) bool { - if p.CommitteeID != committeeProposal.CommitteeID { - return false - } + proposals := k.GetProposalsByCommittee(ctx, committeeProposal.CommitteeID) + for _, p := range proposals { k.DeleteProposalAndVotes(ctx, p.ID) - return false - }) + } k.DeleteCommittee(ctx, committeeProposal.CommitteeID) return nil diff --git a/x/committee/simulation/genesis.go b/x/committee/simulation/genesis.go index 8945a4f5..68ab073b 100644 --- a/x/committee/simulation/genesis.go +++ b/x/committee/simulation/genesis.go @@ -101,7 +101,7 @@ func RandomPermissions(r *rand.Rand, allowedParams []types.AllowedParam) []types allowedParams[i], allowedParams[j] = allowedParams[j], allowedParams[i] }) permissions = append(permissions, - types.ParamChangePermission{ + types.SimpleParamChangePermission{ AllowedParams: allowedParams[:r.Intn(len(allowedParams)+1)], }) } diff --git a/x/committee/simulation/operations.go b/x/committee/simulation/operations.go index 71617aab..f825429f 100644 --- a/x/committee/simulation/operations.go +++ b/x/committee/simulation/operations.go @@ -45,7 +45,7 @@ func WeightedOperations(appParams simulation.AppParams, cdc *codec.Codec, ak Acc wops, simulation.NewWeightedOperation( weight, - SimulateMsgSubmitProposal(ak, k, wContent.ContentSimulatorFn), + SimulateMsgSubmitProposal(cdc, ak, k, wContent.ContentSimulatorFn), ), ) } @@ -55,7 +55,7 @@ func WeightedOperations(appParams simulation.AppParams, cdc *codec.Codec, ak Acc // SimulateMsgSubmitProposal creates a proposal using the passed contentSimulatorFn and tries to find a committee that has permissions for it. If it can't then it uses the fallback committee. // If the fallback committee isn't there (eg when using an non-generated genesis) and no committee can be found this emits a no-op msg and doesn't do anything. // For each submit proposal msg, future ops for the vote messages are generated. Sometimes it doesn't run enough votes to allow the proposal to timeout - the likelihood of this happening is controlled by a parameter. -func SimulateMsgSubmitProposal(ak AccountKeeper, k keeper.Keeper, contentSim simulation.ContentSimulatorFn) simulation.Operation { +func SimulateMsgSubmitProposal(cdc *codec.Codec, ak AccountKeeper, k keeper.Keeper, contentSim simulation.ContentSimulatorFn) simulation.Operation { return func(r *rand.Rand, app *baseapp.BaseApp, ctx sdk.Context, accs []simulation.Account, chainID string) (simulation.OperationMsg, []simulation.FutureOperation, error) { // 1) Send a submit proposal msg @@ -77,7 +77,7 @@ func SimulateMsgSubmitProposal(ak AccountKeeper, k keeper.Keeper, contentSim sim var selectedCommittee types.Committee var found bool for _, c := range committees { - if c.HasPermissionsFor(pp) { + if c.HasPermissionsFor(ctx, cdc, k.ParamKeeper, pp) { selectedCommittee = c found = true break diff --git a/x/committee/simulation/proposals.go b/x/committee/simulation/proposals.go index f59504b7..6d68ddf9 100644 --- a/x/committee/simulation/proposals.go +++ b/x/committee/simulation/proposals.go @@ -125,9 +125,9 @@ func SimulateCommitteeChangeProposalContent(k keeper.Keeper, paramChanges []simu } } -/* // Example custom ParamChangeProposal generator to only generate change to interesting cdp params. // This allows more control over what params are changed within a simulation. +/* func SimulateCDPParamChangeProposalContent(cdpKeeper cdpkeeper.Keeper, paramChangePool []simulation.ParamChange) simulation.ContentSimulatorFn { return func(r *rand.Rand, ctx sdk.Context, _ []simulation.Account) govtypes.Content { @@ -138,7 +138,7 @@ func SimulateCDPParamChangeProposalContent(cdpKeeper cdpkeeper.Keeper, paramChan if len(cp) == 0 { return nil } - cp[0].StabilityFee = sdk.MustNewDecFromStr("0.000001") // TODO generate + cp[0].StabilityFee = sdk.MustNewDecFromStr("0.000001") paramChanges = append( paramChanges, paramstypes.NewParamChange(cdptypes.ModuleName, "?", string(cdptypes.ModuleCdc.MustMarshalJSON(cp))), diff --git a/x/committee/spec/04_events.md b/x/committee/spec/04_events.md index 0e31e71d..bbf79a53 100644 --- a/x/committee/spec/04_events.md +++ b/x/committee/spec/04_events.md @@ -18,9 +18,6 @@ The `x/committee` module emits the following events: | proposal_vote | committee_id | {committee ID} | | proposal_vote | proposal_id | {proposal ID} | | proposal_vote | voter | {voter address} | -| proposal_close | committee_id | {committee ID} | -| proposal_close | proposal_id | {proposal ID} | -| proposal_close | status | {outcome} | | message | module | committee | | message | sender | {sender address} | @@ -30,4 +27,4 @@ The `x/committee` module emits the following events: |----------------------|---------------------|--------------------| | proposal_close | committee_id | {committee ID} | | proposal_close | proposal_id | {proposal ID} | -| proposal_close | status | proposal_timeout | +| proposal_close | status | {outcome} | diff --git a/x/committee/types/codec.go b/x/committee/types/codec.go index dcbaddb6..737f9c8c 100644 --- a/x/committee/types/codec.go +++ b/x/committee/types/codec.go @@ -5,6 +5,7 @@ import ( distrtypes "github.com/cosmos/cosmos-sdk/x/distribution/types" govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" paramstypes "github.com/cosmos/cosmos-sdk/x/params/types" + upgrade "github.com/cosmos/cosmos-sdk/x/upgrade" ) // ModuleCdc is a generic codec to be used throughout module @@ -21,6 +22,8 @@ func init() { RegisterProposalTypeCodec(distrtypes.CommunityPoolSpendProposal{}, "cosmos-sdk/CommunityPoolSpendProposal") RegisterProposalTypeCodec(paramstypes.ParameterChangeProposal{}, "cosmos-sdk/ParameterChangeProposal") RegisterProposalTypeCodec(govtypes.TextProposal{}, "cosmos-sdk/TextProposal") + RegisterProposalTypeCodec(upgrade.SoftwareUpgradeProposal{}, "cosmos-sdk/SoftwareUpgradeProposal") + RegisterProposalTypeCodec(upgrade.CancelSoftwareUpgradeProposal{}, "cosmos-sdk/CancelSoftwareUpgradeProposal") } // RegisterCodec registers the necessary types for the module @@ -34,8 +37,10 @@ func RegisterCodec(cdc *codec.Codec) { // Permissions cdc.RegisterInterface((*Permission)(nil), nil) cdc.RegisterConcrete(GodPermission{}, "kava/GodPermission", nil) - cdc.RegisterConcrete(ParamChangePermission{}, "kava/ParamChangePermission", nil) + cdc.RegisterConcrete(SimpleParamChangePermission{}, "kava/SimpleParamChangePermission", nil) cdc.RegisterConcrete(TextPermission{}, "kava/TextPermission", nil) + cdc.RegisterConcrete(SoftwareUpgradePermission{}, "kava/SoftwareUpgradePermission", nil) + cdc.RegisterConcrete(SubParamChangePermission{}, "kava/SubParamChangePermission", nil) // Msgs cdc.RegisterConcrete(MsgSubmitProposal{}, "kava/MsgSubmitProposal", nil) diff --git a/x/committee/types/committee.go b/x/committee/types/committee.go index 4d5a8d04..f3afaa28 100644 --- a/x/committee/types/committee.go +++ b/x/committee/types/committee.go @@ -6,6 +6,7 @@ import ( yaml "gopkg.in/yaml.v2" + "github.com/cosmos/cosmos-sdk/codec" sdk "github.com/cosmos/cosmos-sdk/types" govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" ) @@ -48,9 +49,9 @@ func (c Committee) HasMember(addr sdk.AccAddress) bool { // HasPermissionsFor returns whether the committee is authorized to enact a proposal. // As long as one permission allows the proposal then it goes through. Its the OR of all permissions. -func (c Committee) HasPermissionsFor(proposal PubProposal) bool { +func (c Committee) HasPermissionsFor(ctx sdk.Context, appCdc *codec.Codec, pk ParamKeeper, proposal PubProposal) bool { for _, p := range c.Permissions { - if p.Allows(proposal) { + if p.Allows(ctx, appCdc, pk, proposal) { return true } } @@ -80,6 +81,12 @@ func (c Committee) Validate() error { return fmt.Errorf("description length %d longer than max allowed %d", len(c.Description), MaxCommitteeDescriptionLength) } + for _, p := range c.Permissions { + if p == nil { + return fmt.Errorf("committee cannot have a nil permission") + } + } + // threshold must be in the range (0,1] if c.VoteThreshold.IsNil() || c.VoteThreshold.LTE(sdk.ZeroDec()) || c.VoteThreshold.GT(sdk.NewDec(1)) { return fmt.Errorf("invalid threshold: %s", c.VoteThreshold) @@ -138,3 +145,17 @@ type Vote struct { ProposalID uint64 `json:"proposal_id" yaml:"proposal_id"` Voter sdk.AccAddress `json:"voter" yaml:"voter"` } + +func NewVote(proposalID uint64, voter sdk.AccAddress) Vote { + return Vote{ + ProposalID: proposalID, + Voter: voter, + } +} + +func (v Vote) Validate() error { + if v.Voter.Empty() { + return fmt.Errorf("voter address cannot be empty") + } + return nil +} diff --git a/x/committee/types/expected_keepers.go b/x/committee/types/expected_keepers.go new file mode 100644 index 00000000..88d02e5d --- /dev/null +++ b/x/committee/types/expected_keepers.go @@ -0,0 +1,9 @@ +package types + +import ( + "github.com/cosmos/cosmos-sdk/x/params" +) + +type ParamKeeper interface { + GetSubspace(string) (params.Subspace, bool) +} diff --git a/x/committee/types/genesis.go b/x/committee/types/genesis.go index 6ee3c4ce..7205ac02 100644 --- a/x/committee/types/genesis.go +++ b/x/committee/types/genesis.go @@ -92,14 +92,15 @@ func (gs GenesisState) Validate() error { // validate votes for _, v := range gs.Votes { + // validate committee + if err := v.Validate(); err != nil { + return err + } + // check proposal exists if !proposalMap[v.ProposalID] { return fmt.Errorf("vote refers to non existent proposal; vote: %+v", v) } - // validate address - if v.Voter.Empty() { - return fmt.Errorf("found empty voter address; vote: %+v", v) - } } return nil } diff --git a/x/committee/types/genesis_test.go b/x/committee/types/genesis_test.go index 3cfe64e4..f3bb4cda 100644 --- a/x/committee/types/genesis_test.go +++ b/x/committee/types/genesis_test.go @@ -12,7 +12,6 @@ import ( "github.com/tendermint/tendermint/crypto" ) -func d(s string) sdk.Dec { return sdk.MustNewDecFromStr(s) } func TestGenesisState_Validate(t *testing.T) { testTime := time.Date(1998, time.January, 1, 0, 0, 0, 0, time.UTC) addresses := []sdk.AccAddress{ diff --git a/x/committee/types/param_permissions_test.go b/x/committee/types/param_permissions_test.go new file mode 100644 index 00000000..07d8aa2d --- /dev/null +++ b/x/committee/types/param_permissions_test.go @@ -0,0 +1,741 @@ +package types + +import ( + sdk "github.com/cosmos/cosmos-sdk/types" + bep3types "github.com/kava-labs/kava/x/bep3/types" + cdptypes "github.com/kava-labs/kava/x/cdp/types" + pricefeedtypes "github.com/kava-labs/kava/x/pricefeed/types" +) + +// Avoid cluttering test cases with long function names +func i(in int64) sdk.Int { return sdk.NewInt(in) } +func d(str string) sdk.Dec { return sdk.MustNewDecFromStr(str) } +func c(denom string, amount int64) sdk.Coin { return sdk.NewInt64Coin(denom, amount) } +func cs(coins ...sdk.Coin) sdk.Coins { return sdk.NewCoins(coins...) } + +func (suite *PermissionsTestSuite) TestAllowedCollateralParams_Allows() { + testCPs := cdptypes.CollateralParams{ + { + Denom: "bnb", + LiquidationRatio: d("2.0"), + DebtLimit: c("usdx", 1000000000000), + StabilityFee: d("1.000000001547125958"), + LiquidationPenalty: d("0.05"), + AuctionSize: i(100), + Prefix: 0x20, + ConversionFactor: i(6), + MarketID: "bnb:usd", + }, + { + Denom: "btc", + LiquidationRatio: d("1.5"), + DebtLimit: c("usdx", 1000000000), + StabilityFee: d("1.000000001547125958"), + LiquidationPenalty: d("0.10"), + AuctionSize: i(1000), + Prefix: 0x30, + ConversionFactor: i(8), + MarketID: "btc:usd", + }, + { + Denom: "atom", + LiquidationRatio: d("2.0"), + DebtLimit: c("usdx", 1000000000), + StabilityFee: d("1.000000001547125958"), + LiquidationPenalty: d("0.07"), + AuctionSize: i(100), + Prefix: 0x40, + ConversionFactor: i(6), + MarketID: "atom:usd", + }, + } + updatedTestCPs := make(cdptypes.CollateralParams, len(testCPs)) + updatedTestCPs[0] = testCPs[1] + updatedTestCPs[1] = testCPs[0] + updatedTestCPs[2] = testCPs[2] + + updatedTestCPs[0].DebtLimit = c("usdx", 1000) // btc + updatedTestCPs[1].LiquidationPenalty = d("0.15") // bnb + updatedTestCPs[2].DebtLimit = c("usdx", 1000) // atom + updatedTestCPs[2].LiquidationPenalty = d("0.15") // atom + + testcases := []struct { + name string + allowed AllowedCollateralParams + current cdptypes.CollateralParams + incoming cdptypes.CollateralParams + expectAllowed bool + }{ + { + name: "disallowed add", + allowed: AllowedCollateralParams{ + { + Denom: "bnb", + AuctionSize: true, + }, + { + Denom: "btc", + StabilityFee: true, + }, + { // allow all fields + Denom: "atom", + LiquidationRatio: true, + DebtLimit: true, + StabilityFee: true, + AuctionSize: true, + LiquidationPenalty: true, + Prefix: true, + MarketID: true, + ConversionFactor: true, + }, + }, + current: testCPs[:2], + incoming: testCPs[:3], + expectAllowed: false, + }, + { + name: "disallowed remove", + allowed: AllowedCollateralParams{ + { + Denom: "bnb", + AuctionSize: true, + }, + { + // allow all fields + Denom: "btc", + LiquidationRatio: true, + DebtLimit: true, + StabilityFee: true, + AuctionSize: true, + LiquidationPenalty: true, + Prefix: true, + MarketID: true, + ConversionFactor: true, + }, + }, + current: testCPs[:2], + incoming: testCPs[:1], // removes btc + expectAllowed: false, + }, + { + name: "allowed change with different order", + allowed: AllowedCollateralParams{ + { + Denom: "bnb", + LiquidationPenalty: true, + }, + { + Denom: "btc", + DebtLimit: true, + }, + { + Denom: "atom", + DebtLimit: true, + LiquidationPenalty: true, + }, + }, + current: testCPs, + incoming: updatedTestCPs, + expectAllowed: true, + }, + } + for _, tc := range testcases { + suite.Run(tc.name, func() { + suite.Require().Equal( + tc.expectAllowed, + tc.allowed.Allows(tc.current, tc.incoming), + ) + }) + } +} + +func (suite *PermissionsTestSuite) TestAllowedAssetParams_Allows() { + testAPs := bep3types.AssetParams{ + { + Denom: "bnb", + CoinID: 714, + Limit: i(1000000000000), + Active: true, + }, + { + Denom: "btc", + CoinID: 0, + Limit: i(1000000000000), + Active: true, + }, + { + Denom: "xrp", + CoinID: 144, + Limit: i(1000000000000), + Active: true, + }, + } + updatedTestAPs := make(bep3types.AssetParams, len(testAPs)) + updatedTestAPs[0] = testAPs[1] + updatedTestAPs[1] = testAPs[0] + updatedTestAPs[2] = testAPs[2] + + updatedTestAPs[0].Limit = i(1000) // btc + updatedTestAPs[1].Active = false // bnb + updatedTestAPs[2].Limit = i(1000) // xrp + updatedTestAPs[2].Active = false // xrp + + testcases := []struct { + name string + allowed AllowedAssetParams + current bep3types.AssetParams + incoming bep3types.AssetParams + expectAllowed bool + }{ + { + name: "disallowed add", + allowed: AllowedAssetParams{ + { + Denom: "bnb", + Active: true, + }, + { + Denom: "btc", + Limit: true, + }, + { // allow all fields + Denom: "xrp", + CoinID: true, + Limit: true, + Active: true, + }, + }, + current: testAPs[:2], + incoming: testAPs[:3], + expectAllowed: false, + }, + { + name: "disallowed remove", + allowed: AllowedAssetParams{ + { + Denom: "bnb", + Active: true, + }, + { // allow all fields + Denom: "btc", + CoinID: true, + Limit: true, + Active: true, + }, + }, + current: testAPs[:2], + incoming: testAPs[:1], // removes btc + expectAllowed: false, + }, + { + name: "allowed change with different order", + allowed: AllowedAssetParams{ + { + Denom: "bnb", + Active: true, + }, + { + Denom: "btc", + Limit: true, + }, + { + Denom: "xrp", + Limit: true, + Active: true, + }, + }, + current: testAPs, + incoming: updatedTestAPs, + expectAllowed: true, + }, + } + for _, tc := range testcases { + suite.Run(tc.name, func() { + suite.Require().Equal( + tc.expectAllowed, + tc.allowed.Allows(tc.current, tc.incoming), + ) + }) + } +} + +func (suite *PermissionsTestSuite) TestAllowedMarkets_Allows() { + testMs := pricefeedtypes.Markets{ + { + MarketID: "bnb:usd", + BaseAsset: "bnb", + QuoteAsset: "usd", + Oracles: []sdk.AccAddress{}, + Active: true, + }, + { + MarketID: "btc:usd", + BaseAsset: "btc", + QuoteAsset: "usd", + Oracles: []sdk.AccAddress{}, + Active: true, + }, + { + MarketID: "atom:usd", + BaseAsset: "atom", + QuoteAsset: "usd", + Oracles: []sdk.AccAddress{}, + Active: true, + }, + } + updatedTestMs := make(pricefeedtypes.Markets, len(testMs)) + updatedTestMs[0] = testMs[1] + updatedTestMs[1] = testMs[0] + updatedTestMs[2] = testMs[2] + + updatedTestMs[0].Oracles = []sdk.AccAddress{[]byte("a test address")} // btc + updatedTestMs[1].Active = false // bnb + updatedTestMs[2].Oracles = []sdk.AccAddress{[]byte("a test address")} // atom + updatedTestMs[2].Active = false // atom + + testcases := []struct { + name string + allowed AllowedMarkets + current pricefeedtypes.Markets + incoming pricefeedtypes.Markets + expectAllowed bool + }{ + { + name: "disallowed add", + allowed: AllowedMarkets{ + { + MarketID: "bnb:usd", + Active: true, + }, + { + MarketID: "btc:usd", + Oracles: true, + }, + { // allow all fields + MarketID: "atom:usd", + BaseAsset: true, + QuoteAsset: true, + Oracles: true, + Active: true, + }, + }, + current: testMs[:2], + incoming: testMs[:3], + expectAllowed: false, + }, + { + name: "disallowed remove", + allowed: AllowedMarkets{ + { + MarketID: "bnb:usd", + Active: true, + }, + { // allow all fields + MarketID: "btc:usd", + BaseAsset: true, + QuoteAsset: true, + Oracles: true, + Active: true, + }, + }, + current: testMs[:2], + incoming: testMs[:1], // removes btc + expectAllowed: false, + }, + { + name: "allowed change with different order", + allowed: AllowedMarkets{ + { + MarketID: "bnb:usd", + Active: true, + }, + { + MarketID: "btc:usd", + Oracles: true, + }, + { + MarketID: "atom:usd", + Oracles: true, + Active: true, + }, + }, + current: testMs, + incoming: updatedTestMs, + expectAllowed: true, + }, + } + for _, tc := range testcases { + suite.Run(tc.name, func() { + suite.Require().Equal( + tc.expectAllowed, + tc.allowed.Allows(tc.current, tc.incoming), + ) + }) + } +} + +func (suite *PermissionsTestSuite) TestAllowedCollateralParam_Allows() { + testCP := cdptypes.CollateralParam{ + Denom: "bnb", + LiquidationRatio: d("1.5"), + DebtLimit: c("usdx", 1000000000000), + StabilityFee: d("1.000000001547125958"), // %5 apr + LiquidationPenalty: d("0.05"), + AuctionSize: i(100), + Prefix: 0x20, + ConversionFactor: i(6), + MarketID: "bnb:usd", + } + newMarketIDCP := testCP + newMarketIDCP.MarketID = "btc:usd" + + newDebtLimitCP := testCP + newDebtLimitCP.DebtLimit = c("usdx", 1000) + + newMarketIDAndDebtLimitCP := testCP + newMarketIDCP.MarketID = "btc:usd" + newDebtLimitCP.DebtLimit = c("usdx", 1000) + + testcases := []struct { + name string + allowed AllowedCollateralParam + current cdptypes.CollateralParam + incoming cdptypes.CollateralParam + expectAllowed bool + }{ + { + name: "allowed change", + allowed: AllowedCollateralParam{ + Denom: "bnb", + DebtLimit: true, + StabilityFee: true, + AuctionSize: true, + }, + current: testCP, + incoming: newDebtLimitCP, + expectAllowed: true, + }, + { + name: "un-allowed change", + allowed: AllowedCollateralParam{ + Denom: "bnb", + DebtLimit: true, + StabilityFee: true, + AuctionSize: true, + }, + current: testCP, + incoming: newMarketIDCP, + expectAllowed: false, + }, + { + name: "un-allowed mismatching denom", + allowed: AllowedCollateralParam{ + Denom: "btc", + DebtLimit: true, + }, + current: testCP, + incoming: newDebtLimitCP, + expectAllowed: false, + }, + + { + name: "allowed no change", + allowed: AllowedCollateralParam{ + Denom: "bnb", + DebtLimit: true, + }, + current: testCP, + incoming: testCP, // no change + expectAllowed: true, + }, + { + name: "un-allowed change with allowed change", + allowed: AllowedCollateralParam{ + Denom: "btc", + DebtLimit: true, + }, + current: testCP, + incoming: newMarketIDAndDebtLimitCP, + expectAllowed: false, + }, + // TODO { + // name: "nil Int values", + // allowed: AllowedCollateralParam{ + // Denom: "btc", + // DebtLimit: true, + // }, + // incoming: cdptypes.CollateralParam{}, // nil sdk.Int types + // current: testCP, + // expectAllowed: false, + // }, + } + + for _, tc := range testcases { + suite.Run(tc.name, func() { + suite.Require().Equal( + tc.expectAllowed, + tc.allowed.Allows(tc.current, tc.incoming), + ) + }) + } +} + +func (suite *PermissionsTestSuite) TestAllowedDebtParam_Allows() { + testDP := cdptypes.DebtParam{ + Denom: "usdx", + ReferenceAsset: "usd", + ConversionFactor: i(6), + DebtFloor: i(10000000), + SavingsRate: d("0.95"), + } + newDenomDP := testDP + newDenomDP.Denom = "usdz" + + newDebtFloorDP := testDP + newDebtFloorDP.DebtFloor = i(1000) + + newDenomAndDebtFloorDP := testDP + newDenomAndDebtFloorDP.Denom = "usdz" + newDenomAndDebtFloorDP.DebtFloor = i(1000) + + testcases := []struct { + name string + allowed AllowedDebtParam + current cdptypes.DebtParam + incoming cdptypes.DebtParam + expectAllowed bool + }{ + { + name: "allowed change", + allowed: AllowedDebtParam{ + DebtFloor: true, + SavingsRate: true, + }, + current: testDP, + incoming: newDebtFloorDP, + expectAllowed: true, + }, + { + name: "un-allowed change", + allowed: AllowedDebtParam{ + DebtFloor: true, + SavingsRate: true, + }, + current: testDP, + incoming: newDenomDP, + expectAllowed: false, + }, + { + name: "allowed no change", + allowed: AllowedDebtParam{ + DebtFloor: true, + SavingsRate: true, + }, + current: testDP, + incoming: testDP, // no change + expectAllowed: true, + }, + { + name: "un-allowed change with allowed change", + allowed: AllowedDebtParam{ + DebtFloor: true, + SavingsRate: true, + }, + current: testDP, + incoming: newDenomAndDebtFloorDP, + expectAllowed: false, + }, + // TODO { + // name: "nil Int values", + // allowed: AllowedCollateralParam{ + // Denom: "btc", + // DebtLimit: true, + // }, + // incoming: cdptypes.CollateralParam{}, // nil sdk.Int types + // current: testCP, + // expectAllowed: false, + // }, + } + + for _, tc := range testcases { + suite.Run(tc.name, func() { + suite.Require().Equal( + tc.expectAllowed, + tc.allowed.Allows(tc.current, tc.incoming), + ) + }) + } +} + +func (suite *PermissionsTestSuite) TestAllowedAssetParam_Allows() { + testAP := bep3types.AssetParam{ + Denom: "usdx", + CoinID: 999, + Limit: i(1000000000), + Active: true, + } + newCoinidAP := testAP + newCoinidAP.CoinID = 0 + + newLimitAP := testAP + newLimitAP.Limit = i(1000) + + newCoinidAndLimitAP := testAP + newCoinidAndLimitAP.CoinID = 0 + newCoinidAndLimitAP.Limit = i(1000) + + testcases := []struct { + name string + allowed AllowedAssetParam + current bep3types.AssetParam + incoming bep3types.AssetParam + expectAllowed bool + }{ + { + name: "allowed change", + allowed: AllowedAssetParam{ + Denom: "usdx", + Limit: true, + }, + current: testAP, + incoming: newLimitAP, + expectAllowed: true, + }, + { + name: "un-allowed change", + allowed: AllowedAssetParam{ + Denom: "usdx", + Limit: true, + }, + current: testAP, + incoming: newCoinidAP, + expectAllowed: false, + }, + { + name: "allowed no change", + allowed: AllowedAssetParam{ + Denom: "usdx", + Limit: true, + }, + current: testAP, + incoming: testAP, // no change + expectAllowed: true, + }, + { + name: "un-allowed change with allowed change", + allowed: AllowedAssetParam{ + Denom: "usdx", + Limit: true, + }, + current: testAP, + incoming: newCoinidAndLimitAP, + expectAllowed: false, + }, + // TODO { + // name: "nil Int values", + // allowed: AllowedCollateralParam{ + // Denom: "btc", + // DebtLimit: true, + // }, + // incoming: cdptypes.CollateralParam{}, // nil sdk.Int types + // current: testCP, + // expectAllowed: false, + // }, + } + + for _, tc := range testcases { + suite.Run(tc.name, func() { + suite.Require().Equal( + tc.expectAllowed, + tc.allowed.Allows(tc.current, tc.incoming), + ) + }) + } +} + +func (suite *PermissionsTestSuite) TestAllowedMarket_Allows() { + testM := pricefeedtypes.Market{ + MarketID: "bnb:usd", + BaseAsset: "bnb", + QuoteAsset: "usd", + Oracles: []sdk.AccAddress{[]byte("a test address")}, + Active: true, + } + newOraclesM := testM + newOraclesM.Oracles = nil + + newActiveM := testM + newActiveM.Active = false + + newOraclesAndActiveM := testM + newOraclesAndActiveM.Oracles = nil + newOraclesAndActiveM.Active = false + + testcases := []struct { + name string + allowed AllowedMarket + current pricefeedtypes.Market + incoming pricefeedtypes.Market + expectAllowed bool + }{ + { + name: "allowed change", + allowed: AllowedMarket{ + MarketID: "bnb:usd", + Active: true, + }, + current: testM, + incoming: newActiveM, + expectAllowed: true, + }, + { + name: "un-allowed change", + allowed: AllowedMarket{ + MarketID: "bnb:usd", + Active: true, + }, + current: testM, + incoming: newOraclesM, + expectAllowed: false, + }, + { + name: "allowed no change", + allowed: AllowedMarket{ + MarketID: "bnb:usd", + Active: true, + }, + current: testM, + incoming: testM, // no change + expectAllowed: true, + }, + { + name: "un-allowed change with allowed change", + allowed: AllowedMarket{ + MarketID: "bnb:usd", + Active: true, + }, + current: testM, + incoming: newOraclesAndActiveM, + expectAllowed: false, + }, + // TODO { + // name: "nil Int values", + // allowed: AllowedCollateralParam{ + // Denom: "btc", + // DebtLimit: true, + // }, + // incoming: cdptypes.CollateralParam{}, // nil sdk.Int types + // current: testCP, + // expectAllowed: false, + // }, + } + + for _, tc := range testcases { + suite.Run(tc.name, func() { + suite.Require().Equal( + tc.expectAllowed, + tc.allowed.Allows(tc.current, tc.incoming), + ) + }) + } +} diff --git a/x/committee/types/permissions.go b/x/committee/types/permissions.go index 3cfbd5f9..38491c11 100644 --- a/x/committee/types/permissions.go +++ b/x/committee/types/permissions.go @@ -1,8 +1,16 @@ package types import ( + "github.com/cosmos/cosmos-sdk/codec" + sdk "github.com/cosmos/cosmos-sdk/types" govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" paramstypes "github.com/cosmos/cosmos-sdk/x/params/types" + upgrade "github.com/cosmos/cosmos-sdk/x/upgrade" + + bep3types "github.com/kava-labs/kava/x/bep3/types" + cdptypes "github.com/kava-labs/kava/x/cdp/types" + "github.com/kava-labs/kava/x/pricefeed" + pricefeedtypes "github.com/kava-labs/kava/x/pricefeed/types" ) func init() { @@ -10,13 +18,15 @@ func init() { // But since these proposals contain Permissions, these types also need registering: govtypes.ModuleCdc.RegisterInterface((*Permission)(nil), nil) govtypes.RegisterProposalTypeCodec(GodPermission{}, "kava/GodPermission") - govtypes.RegisterProposalTypeCodec(ParamChangePermission{}, "kava/ParamChangePermission") + govtypes.RegisterProposalTypeCodec(SimpleParamChangePermission{}, "kava/SimpleParamChangePermission") govtypes.RegisterProposalTypeCodec(TextPermission{}, "kava/TextPermission") + govtypes.RegisterProposalTypeCodec(SoftwareUpgradePermission{}, "kava/SoftwareUpgradePermission") + govtypes.RegisterProposalTypeCodec(SubParamChangePermission{}, "kava/SubParamChangePermission") } // Permission is anything with a method that validates whether a proposal is allowed by it or not. type Permission interface { - Allows(PubProposal) bool + Allows(sdk.Context, *codec.Codec, ParamKeeper, PubProposal) bool } // ------------------------------------------ @@ -28,7 +38,7 @@ type GodPermission struct{} var _ Permission = GodPermission{} -func (GodPermission) Allows(PubProposal) bool { return true } +func (GodPermission) Allows(sdk.Context, *codec.Codec, ParamKeeper, PubProposal) bool { return true } func (GodPermission) MarshalYAML() (interface{}, error) { valueToMarshal := struct { @@ -40,17 +50,17 @@ func (GodPermission) MarshalYAML() (interface{}, error) { } // ------------------------------------------ -// ParamChangePermission +// SimpleParamChangePermission // ------------------------------------------ -// ParamChangeProposal only allows changes to certain params -type ParamChangePermission struct { +// SimpleParamChangePermission only allows changes to certain params +type SimpleParamChangePermission struct { AllowedParams AllowedParams `json:"allowed_params" yaml:"allowed_params"` } -var _ Permission = ParamChangePermission{} +var _ Permission = SimpleParamChangePermission{} -func (perm ParamChangePermission) Allows(p PubProposal) bool { +func (perm SimpleParamChangePermission) Allows(_ sdk.Context, _ *codec.Codec, _ ParamKeeper, p PubProposal) bool { proposal, ok := p.(paramstypes.ParameterChangeProposal) if !ok { return false @@ -63,7 +73,7 @@ func (perm ParamChangePermission) Allows(p PubProposal) bool { return true } -func (perm ParamChangePermission) MarshalYAML() (interface{}, error) { +func (perm SimpleParamChangePermission) MarshalYAML() (interface{}, error) { valueToMarshal := struct { Type string `yaml:"type"` AllowedParams AllowedParams `yaml:"allowed_params"` @@ -98,7 +108,7 @@ type TextPermission struct{} var _ Permission = TextPermission{} -func (TextPermission) Allows(p PubProposal) bool { +func (TextPermission) Allows(_ sdk.Context, _ *codec.Codec, _ ParamKeeper, p PubProposal) bool { _, ok := p.(govtypes.TextProposal) return ok } @@ -111,3 +121,438 @@ func (TextPermission) MarshalYAML() (interface{}, error) { } return valueToMarshal, nil } + +// ------------------------------------------ +// SoftwareUpgradePermission +// ------------------------------------------ + +type SoftwareUpgradePermission struct{} + +var _ Permission = SoftwareUpgradePermission{} + +func (SoftwareUpgradePermission) Allows(_ sdk.Context, _ *codec.Codec, _ ParamKeeper, p PubProposal) bool { + _, ok := p.(upgrade.SoftwareUpgradeProposal) + return ok +} + +func (SoftwareUpgradePermission) MarshalYAML() (interface{}, error) { + valueToMarshal := struct { + Type string `yaml:"type"` + }{ + Type: "software_upgrade_permission", + } + return valueToMarshal, nil +} + +// ------------------------------------------ +// SubParamChangePermission +// ------------------------------------------ + +// ParamChangeProposal only allows changes to certain params +type SubParamChangePermission struct { + AllowedParams AllowedParams `json:"allowed_params" yaml:"allowed_params"` + AllowedCollateralParams AllowedCollateralParams `json:"allowed_collateral_params" yaml:"allowed_collateral_params"` + AllowedDebtParam AllowedDebtParam `json:"allowed_debt_param" yaml:"allowed_debt_param"` + AllowedAssetParams AllowedAssetParams `json:"allowed_asset_params" yaml:"allowed_asset_params"` + AllowedMarkets AllowedMarkets `json:"allowed_markets" yaml:"allowed_markets"` +} + +var _ Permission = SubParamChangePermission{} + +func (perm SubParamChangePermission) MarshalYAML() (interface{}, error) { + valueToMarshal := struct { + Type string `yaml:"type"` + AllowedParams AllowedParams `yaml:"allowed_params"` + AllowedCollateralParams AllowedCollateralParams `yaml:"allowed_collateral_params"` + AllowedDebtParam AllowedDebtParam `yaml:"allowed_debt_param"` + AllowedAssetParams AllowedAssetParams `yaml:"allowed_asset_params"` + AllowedMarkets AllowedMarkets `yaml:"allowed_markets"` + }{ + Type: "param_change_permission", + AllowedParams: perm.AllowedParams, + AllowedCollateralParams: perm.AllowedCollateralParams, + AllowedDebtParam: perm.AllowedDebtParam, + AllowedAssetParams: perm.AllowedAssetParams, + AllowedMarkets: perm.AllowedMarkets, + } + return valueToMarshal, nil +} + +func (perm SubParamChangePermission) Allows(ctx sdk.Context, appCdc *codec.Codec, pk ParamKeeper, p PubProposal) bool { + // Check pubproposal has correct type + proposal, ok := p.(paramstypes.ParameterChangeProposal) + if !ok { + return false + } + // Check the param changes match the allowed keys + for _, change := range proposal.Changes { + if !perm.AllowedParams.Contains(change) { + return false + } + } + // Check any CollateralParam changes are allowed + + // Get the incoming CollaterParams value + var foundIncomingCP bool + var incomingCP cdptypes.CollateralParams + for _, change := range proposal.Changes { + if !(change.Subspace == cdptypes.ModuleName && change.Key == string(cdptypes.KeyCollateralParams)) { + continue + } + // note: in case of duplicates take the last value + foundIncomingCP = true + if err := appCdc.UnmarshalJSON([]byte(change.Value), &incomingCP); err != nil { + return false // invalid json value, so just disallow + } + } + // only check if there was a proposed change + if foundIncomingCP { + // Get the current value of the CollateralParams + cdpSubspace, found := pk.GetSubspace(cdptypes.ModuleName) + if !found { + return false // not using a panic to help avoid begin blocker panics + } + var currentCP cdptypes.CollateralParams + cdpSubspace.Get(ctx, cdptypes.KeyCollateralParams, ¤tCP) // panics if something goes wrong + + // Check all the incoming changes in the CollateralParams are allowed + collateralParamChangesAllowed := perm.AllowedCollateralParams.Allows(currentCP, incomingCP) + if !collateralParamChangesAllowed { + return false + } + } + + // Check any DebtParam changes are allowed + + // Get the incoming DebtParam value + var foundIncomingDP bool + var incomingDP cdptypes.DebtParam + for _, change := range proposal.Changes { + if !(change.Subspace == cdptypes.ModuleName && change.Key == string(cdptypes.KeyDebtParam)) { + continue + } + // note: in case of duplicates take the last value + foundIncomingDP = true + if err := appCdc.UnmarshalJSON([]byte(change.Value), &incomingDP); err != nil { + return false // invalid json value, so just disallow + } + } + // only check if there was a proposed change + if foundIncomingDP { + // Get the current value of the DebtParams + cdpSubspace, found := pk.GetSubspace(cdptypes.ModuleName) + if !found { + return false // not using a panic to help avoid begin blocker panics + } + var currentDP cdptypes.DebtParam + cdpSubspace.Get(ctx, cdptypes.KeyDebtParam, ¤tDP) // panics if something goes wrong + + // Check the incoming changes in the DebtParam are allowed + debtParamChangeAllowed := perm.AllowedDebtParam.Allows(currentDP, incomingDP) + if !debtParamChangeAllowed { + return false + } + } + + // Check any AssetParams changes are allowed + + // Get the incoming AssetParams value + var foundIncomingAPs bool + var incomingAPs bep3types.AssetParams + for _, change := range proposal.Changes { + if !(change.Subspace == bep3types.ModuleName && change.Key == string(bep3types.KeySupportedAssets)) { + continue + } + // note: in case of duplicates take the last value + foundIncomingAPs = true + if err := appCdc.UnmarshalJSON([]byte(change.Value), &incomingAPs); err != nil { + return false // invalid json value, so just disallow + } + } + // only check if there was a proposed change + if foundIncomingAPs { + // Get the current value of the SupportedAssets + subspace, found := pk.GetSubspace(bep3types.ModuleName) + if !found { + return false // not using a panic to help avoid begin blocker panics + } + var currentAPs bep3types.AssetParams + subspace.Get(ctx, bep3types.KeySupportedAssets, ¤tAPs) // panics if something goes wrong + + // Check all the incoming changes in the CollateralParams are allowed + assetParamsChangesAllowed := perm.AllowedAssetParams.Allows(currentAPs, incomingAPs) + if !assetParamsChangesAllowed { + return false + } + } + + // Check any Markets changes are allowed + + // Get the incoming Markets value + var foundIncomingMs bool + var incomingMs pricefeedtypes.Markets + for _, change := range proposal.Changes { + if !(change.Subspace == pricefeedtypes.ModuleName && change.Key == string(pricefeedtypes.KeyMarkets)) { + continue + } + // note: in case of duplicates take the last value + foundIncomingMs = true + if err := appCdc.UnmarshalJSON([]byte(change.Value), &incomingMs); err != nil { + return false // invalid json value, so just disallow + } + } + // only check if there was a proposed change + if foundIncomingMs { + // Get the current value of the Markets + subspace, found := pk.GetSubspace(pricefeedtypes.ModuleName) + if !found { + return false // not using a panic to help avoid begin blocker panics + } + var currentMs pricefeedtypes.Markets + subspace.Get(ctx, pricefeedtypes.KeyMarkets, ¤tMs) // panics if something goes wrong + + // Check all the incoming changes in the Markets are allowed + marketsChangesAllowed := perm.AllowedMarkets.Allows(currentMs, incomingMs) + if !marketsChangesAllowed { + return false + } + } + + return true +} + +type AllowedCollateralParams []AllowedCollateralParam + +func (acps AllowedCollateralParams) Allows(current, incoming cdptypes.CollateralParams) bool { + allAllowed := true + + // do not allow CollateralParams to be added or removed + // this checks both lists are the same size, then below checks each incoming matches a current + if len(incoming) != len(current) { + return false + } + + // for each param struct, check it is allowed, and if it is not, check the value has not changed + for _, incomingCP := range incoming { + // 1) check incoming cp is in list of allowed cps + var foundAllowedCP bool + var allowedCP AllowedCollateralParam + for _, p := range acps { + if p.Denom != incomingCP.Denom { + continue + } + foundAllowedCP = true + allowedCP = p + } + if !foundAllowedCP { + // incoming had a CollateralParam that wasn't in the list of allowed ones + return false + } + + // 2) Check incoming changes are individually allowed + // find existing CollateralParam + var foundCurrentCP bool + var currentCP cdptypes.CollateralParam + for _, p := range current { + if p.Denom != incomingCP.Denom { + continue + } + foundCurrentCP = true + currentCP = p + } + if !foundCurrentCP { + return false // not allowed to add param to list + } + // check changed values are all allowed + allowed := allowedCP.Allows(currentCP, incomingCP) + + allAllowed = allAllowed && allowed + } + return allAllowed +} + +type AllowedCollateralParam struct { + Denom string `json:"denom" yaml:"denom"` + LiquidationRatio bool `json:"liquidation_ratio" yaml:"liquidation_ratio"` + DebtLimit bool `json:"debt_limit" yaml:"debt_limit"` + StabilityFee bool `json:"stability_fee" yaml:"stability_fee"` + AuctionSize bool `json:"auction_size" yaml:"auction_size"` + LiquidationPenalty bool `json:"liquidation_penalty" yaml:"liquidation_penalty"` + Prefix bool `json:"prefix" yaml:"prefix"` + MarketID bool `json:"market_id" yaml:"market_id"` + ConversionFactor bool `json:"conversion_factor" yaml:"conversion_factor"` +} + +func (acp AllowedCollateralParam) Allows(current, incoming cdptypes.CollateralParam) bool { + allowed := ((acp.Denom == current.Denom) && (acp.Denom == incoming.Denom)) && // require denoms to be all equal + (current.LiquidationRatio.Equal(incoming.LiquidationRatio) || acp.LiquidationRatio) && + (current.DebtLimit.IsEqual(incoming.DebtLimit) || acp.DebtLimit) && + (current.StabilityFee.Equal(incoming.StabilityFee) || acp.StabilityFee) && + (current.AuctionSize.Equal(incoming.AuctionSize) || acp.AuctionSize) && + (current.LiquidationPenalty.Equal(incoming.LiquidationPenalty) || acp.LiquidationPenalty) && + ((current.Prefix == incoming.Prefix) || acp.Prefix) && + ((current.MarketID == incoming.MarketID) || acp.MarketID) && + (current.ConversionFactor.Equal(incoming.ConversionFactor) || acp.ConversionFactor) + return allowed +} + +type AllowedDebtParam struct { + Denom bool `json:"denom" yaml:"denom"` + ReferenceAsset bool `json:"reference_asset" yaml:"reference_asset"` + ConversionFactor bool `json:"conversion_factor" yaml:"conversion_factor"` + DebtFloor bool `json:"debt_floor" yaml:"debt_floor"` + SavingsRate bool `json:"savings_rate" yaml:"savings_rate"` +} + +func (adp AllowedDebtParam) Allows(current, incoming cdptypes.DebtParam) bool { + allowed := ((current.Denom == incoming.Denom) || adp.Denom) && + ((current.ReferenceAsset == incoming.ReferenceAsset) || adp.ReferenceAsset) && + (current.ConversionFactor.Equal(incoming.ConversionFactor) || adp.ConversionFactor) && + (current.DebtFloor.Equal(incoming.DebtFloor) || adp.DebtFloor) && + (current.SavingsRate.Equal(incoming.SavingsRate) || adp.SavingsRate) + return allowed +} + +type AllowedAssetParams []AllowedAssetParam + +func (aaps AllowedAssetParams) Allows(current, incoming bep3types.AssetParams) bool { + allAllowed := true + + // do not allow AssetParams to be added or removed + // this checks both lists are the same size, then below checks each incoming matches a current + if len(incoming) != len(current) { + return false + } + + // for each asset struct, check it is allowed, and if it is not, check the value has not changed + for _, incomingAP := range incoming { + // 1) check incoming ap is in list of allowed aps + var foundAllowedAP bool + var allowedAP AllowedAssetParam + for _, p := range aaps { + if p.Denom != incomingAP.Denom { + continue + } + foundAllowedAP = true + allowedAP = p + } + if !foundAllowedAP { + // incoming had a AssetParam that wasn't in the list of allowed ones + return false + } + + // 2) Check incoming changes are individually allowed + // find existing SupportedAsset + var foundCurrentAP bool + var currentAP bep3types.AssetParam + for _, p := range current { + if p.Denom != incomingAP.Denom { + continue + } + foundCurrentAP = true + currentAP = p + } + if !foundCurrentAP { + return false // not allowed to add asset to list + } + // check changed values are all allowed + allowed := allowedAP.Allows(currentAP, incomingAP) + + allAllowed = allAllowed && allowed + } + return allAllowed +} + +type AllowedAssetParam struct { + Denom string `json:"denom" yaml:"denom"` + CoinID bool `json:"coin_id" yaml:"coin_id"` + Limit bool `json:"limit" yaml:"limit"` + Active bool `json:"active" yaml:"active"` +} + +func (aap AllowedAssetParam) Allows(current, incoming bep3types.AssetParam) bool { + allowed := ((aap.Denom == current.Denom) && (aap.Denom == incoming.Denom)) && // require denoms to be all equal + ((current.CoinID == incoming.CoinID) || aap.CoinID) && + (current.Limit.Equal(incoming.Limit) || aap.Limit) && + ((current.Active == incoming.Active) || aap.Active) + return allowed +} + +type AllowedMarkets []AllowedMarket + +func (ams AllowedMarkets) Allows(current, incoming pricefeedtypes.Markets) bool { + allAllowed := true + + // do not allow Markets to be added or removed + // this checks both lists are the same size, then below checks each incoming matches a current + if len(incoming) != len(current) { + return false + } + + // for each market struct, check it is allowed, and if it is not, check the value has not changed + for _, incomingM := range incoming { + // 1) check incoming market is in list of allowed markets + var foundAllowedM bool + var allowedM AllowedMarket + for _, p := range ams { + if p.MarketID != incomingM.MarketID { + continue + } + foundAllowedM = true + allowedM = p + } + if !foundAllowedM { + // incoming had a Market that wasn't in the list of allowed ones + return false + } + + // 2) Check incoming changes are individually allowed + // find existing SupportedAsset + var foundCurrentM bool + var currentM pricefeed.Market + for _, p := range current { + if p.MarketID != incomingM.MarketID { + continue + } + foundCurrentM = true + currentM = p + } + if !foundCurrentM { + return false // not allowed to add market to list + } + // check changed values are all allowed + allowed := allowedM.Allows(currentM, incomingM) + + allAllowed = allAllowed && allowed + } + return allAllowed +} + +type AllowedMarket struct { + MarketID string `json:"market_id" yaml:"market_id"` + BaseAsset bool `json:"base_asset" yaml:"base_asset"` + QuoteAsset bool `json:"quote_asset" yaml:"quote_asset"` + Oracles bool `json:"oracles" yaml:"oracles"` + Active bool `json:"active" yaml:"active"` +} + +func (am AllowedMarket) Allows(current, incoming pricefeedtypes.Market) bool { + allowed := ((am.MarketID == current.MarketID) && (am.MarketID == incoming.MarketID)) && // require denoms to be all equal + ((current.BaseAsset == incoming.BaseAsset) || am.BaseAsset) && + ((current.QuoteAsset == incoming.QuoteAsset) || am.QuoteAsset) && + (addressesEqual(current.Oracles, incoming.Oracles) || am.Oracles) && + ((current.Active == incoming.Active) || am.Active) + return allowed +} + +// addressesEqual check if slices of addresses are equal, the order matters +func addressesEqual(addrs1, addrs2 []sdk.AccAddress) bool { + if len(addrs1) != len(addrs2) { + return false + } + areEqual := true + for i := range addrs1 { + areEqual = areEqual && addrs1[i].Equals(addrs2[i]) + } + return areEqual +} diff --git a/x/committee/types/permissions_test.go b/x/committee/types/permissions_test.go index 54baaa10..bc2dddf1 100644 --- a/x/committee/types/permissions_test.go +++ b/x/committee/types/permissions_test.go @@ -2,11 +2,14 @@ package types import ( "testing" + "time" "github.com/stretchr/testify/suite" + sdk "github.com/cosmos/cosmos-sdk/types" govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" paramstypes "github.com/cosmos/cosmos-sdk/x/params/types" + upgrade "github.com/cosmos/cosmos-sdk/x/upgrade" ) type PermissionsTestSuite struct { @@ -36,7 +39,7 @@ func (suite *PermissionsTestSuite) SetupTest() { } } -func (suite *PermissionsTestSuite) TestParamChangePermission_Allows() { +func (suite *PermissionsTestSuite) TestSimpleParamChangePermission_Allows() { testcases := []struct { name string allowedParams AllowedParams @@ -133,12 +136,12 @@ func (suite *PermissionsTestSuite) TestParamChangePermission_Allows() { for _, tc := range testcases { suite.Run(tc.name, func() { - permission := ParamChangePermission{ + permission := SimpleParamChangePermission{ AllowedParams: tc.allowedParams, } suite.Equal( tc.expectAllowed, - permission.Allows(tc.pubProposal), + permission.Allows(sdk.Context{}, nil, nil, tc.pubProposal), ) }) } @@ -276,7 +279,64 @@ func (suite *PermissionsTestSuite) TestTextPermission_Allows() { permission := TextPermission{} suite.Equal( tc.expectAllowed, - permission.Allows(tc.pubProposal), + permission.Allows(sdk.Context{}, nil, nil, tc.pubProposal), + ) + }) + } +} + +func (suite *PermissionsTestSuite) TestSoftwareUpgradePermission_Allows() { + testcases := []struct { + name string + pubProposal PubProposal + expectAllowed bool + }{ + { + name: "normal", + pubProposal: upgrade.NewSoftwareUpgradeProposal( + "A Title", + "A description for this proposal.", + upgrade.Plan{ + Name: "upgrade v0.12.1", + Time: time.Date(1998, 1, 1, 0, 0, 0, 0, time.UTC), + Info: "some information", + }, + ), + expectAllowed: true, + }, + { + name: "not allowed (wrong pubproposal type)", + pubProposal: paramstypes.NewParameterChangeProposal( + "A Title", + "A description for this proposal.", + []paramstypes.ParamChange{ + { + Subspace: "cdp", + Key: "DebtThreshold", + Value: `{"denom": "usdx", "amount": "1000000"}`, + }, + { + Subspace: "cdp", + Key: "CollateralParams", + Value: `[]`, + }, + }, + ), + expectAllowed: false, + }, + { + name: "not allowed (nil pubproposal)", + pubProposal: nil, + expectAllowed: false, + }, + } + + for _, tc := range testcases { + suite.Run(tc.name, func() { + permission := SoftwareUpgradePermission{} + suite.Equal( + tc.expectAllowed, + permission.Allows(sdk.Context{}, nil, nil, tc.pubProposal), ) }) }