From c843436039b3eada3451856961c82380fafb646b Mon Sep 17 00:00:00 2001 From: Matt Witkowski Date: Fri, 26 Jan 2024 18:30:27 -0500 Subject: [PATCH] Allow groups to force transfer. (#1821) * Add permission checker to see if an account is part of a group for marker transfers. * Bump github.com/google/uuid from 1.5.0 to 1.6.0 (#1819) * Bump github.com/google/uuid from 1.5.0 to 1.6.0 Bumps [github.com/google/uuid](https://github.com/google/uuid) from 1.5.0 to 1.6.0. - [Release notes](https://github.com/google/uuid/releases) - [Changelog](https://github.com/google/uuid/blob/master/CHANGELOG.md) - [Commits](https://github.com/google/uuid/compare/v1.5.0...v1.6.0) --- updated-dependencies: - dependency-name: github.com/google/uuid dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] * Updated Changelog --------- Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: dependabot[bot] * Update changelog entry. * Add temporary logic that checks if an address is part of a groups module. * Fix bug in temporary test code. This solution fixes the issue brought up. * Rename interface to GroupChecker. * Move GroupChecker into group.go * Move group related functions to group.go * Add test for nil address and add tests for TestIsGroupAddress. * Add test for NewGroupChecker. * Add keeper test for canForceTransferFrom for group. --------- Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: dependabot[bot] --- CHANGELOG.md | 1 + app/app.go | 2 + app/group.go | 34 +++++++++++ app/group_test.go | 74 ++++++++++++++++++++++++ x/marker/keeper/keeper.go | 5 ++ x/marker/keeper/keeper_test.go | 23 +++++++- x/marker/keeper/marker.go | 5 ++ x/marker/keeper/proposal_handler_test.go | 2 +- x/marker/simulation/proposals_test.go | 1 + x/marker/types/expected_keepers.go | 5 ++ 10 files changed, 150 insertions(+), 2 deletions(-) create mode 100644 app/group.go create mode 100644 app/group_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index a073255071..e8649000b2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,6 +53,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * Remove deleted marker send deny entries [#1666](https://github.com/provenance-io/provenance/issues/1666). * Update protos, naming, and documentation to use mills [#1813](https://github.com/provenance-io/provenance/issues/1813). +* Update marker transfer to work with groups [#1818](https://github.com/provenance-io/provenance/issues/1818). ### Dependencies diff --git a/app/app.go b/app/app.go index 604376f5d7..e2e33941e7 100644 --- a/app/app.go +++ b/app/app.go @@ -576,10 +576,12 @@ func New( authtypes.NewModuleAddress(stakingtypes.BondedPoolName), // Allow bond denom to be a restricted coin. authtypes.NewModuleAddress(stakingtypes.NotBondedPoolName), // Allow bond denom to be a restricted coin. } + app.MarkerKeeper = markerkeeper.NewKeeper( appCodec, keys[markertypes.StoreKey], app.GetSubspace(markertypes.ModuleName), app.AccountKeeper, app.BankKeeper, app.AuthzKeeper, app.FeeGrantKeeper, app.AttributeKeeper, app.NameKeeper, app.TransferKeeper, markerReqAttrBypassAddrs, + NewGroupCheckerFunc(app.GroupKeeper), ) app.HoldKeeper = holdkeeper.NewKeeper( diff --git a/app/group.go b/app/group.go new file mode 100644 index 0000000000..42e9e46299 --- /dev/null +++ b/app/group.go @@ -0,0 +1,34 @@ +package app + +import ( + "context" + + sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/cosmos/cosmos-sdk/x/group" +) + +// GroupCheckerFunc convenient type to match the GroupChecker interface. +type GroupCheckerFunc func(sdk.Context, sdk.AccAddress) bool + +// GroupPolicyQuerier provides functionality to query group policies. +type GroupPolicyQuerier interface { + GroupPolicyInfo(goCtx context.Context, request *group.QueryGroupPolicyInfoRequest) (*group.QueryGroupPolicyInfoResponse, error) +} + +// IsGroupAddress checks if the account is a group address. +func (t GroupCheckerFunc) IsGroupAddress(ctx sdk.Context, account sdk.AccAddress) bool { + if account == nil { + return false + } + return t(ctx, account) +} + +// NewGroupCheckerFunc creates a new GroupChecker function for checking if an account is in a group. +func NewGroupCheckerFunc(querier GroupPolicyQuerier) GroupCheckerFunc { + return GroupCheckerFunc(func(ctx sdk.Context, account sdk.AccAddress) bool { + msg := &group.QueryGroupPolicyInfoRequest{Address: account.String()} + goCtx := sdk.WrapSDKContext(ctx) + _, err := querier.GroupPolicyInfo(goCtx, msg) + return err == nil + }) +} diff --git a/app/group_test.go b/app/group_test.go new file mode 100644 index 0000000000..76d489c3c1 --- /dev/null +++ b/app/group_test.go @@ -0,0 +1,74 @@ +package app + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + + cerrs "cosmossdk.io/errors" + sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/cosmos/cosmos-sdk/x/group" + tmproto "github.com/tendermint/tendermint/proto/tendermint/types" +) + +func TestNewGroupCheckerFunc(t *testing.T) { + querier := NewMockGroupPolicyQuerier(true) + checker := NewGroupCheckerFunc(querier) + assert.NotNil(t, checker, "should return a group checker function") +} + +func TestIsGroupAddress(t *testing.T) { + tests := []struct { + name string + querySuccess bool + address sdk.AccAddress + }{ + { + name: "should be true with group address", + querySuccess: true, + address: sdk.AccAddress("test"), + }, + { + name: "should return false with non group address", + querySuccess: false, + address: sdk.AccAddress("test"), + }, + { + name: "should return false with nil address", + querySuccess: false, + address: nil, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + querier := NewMockGroupPolicyQuerier(tc.querySuccess) + checker := NewGroupCheckerFunc(querier) + ctx := sdk.NewContext(nil, tmproto.Header{}, true, nil) + success := checker.IsGroupAddress(ctx, tc.address) + assert.Equal(t, tc.querySuccess, success, "should correctly detect if the supplied address is a group address") + }) + } +} + +// MockGroupPolicyQuerier mocks the querier so a GroupKeeper isn't needed. +type MockGroupPolicyQuerier struct { + isGroupAddress bool +} + +// NewMockGroupPolicyQuerier creates a new MockGroupPolicyQuerier. +func NewMockGroupPolicyQuerier(isGroupAddress bool) *MockGroupPolicyQuerier { + return &MockGroupPolicyQuerier{ + isGroupAddress: isGroupAddress, + } +} + +// GroupPolicyInfo provides a stubbed implementation of the GroupPolicyInfo method. +func (t MockGroupPolicyQuerier) GroupPolicyInfo(goCtx context.Context, request *group.QueryGroupPolicyInfoRequest) (*group.QueryGroupPolicyInfoResponse, error) { + var err error + if !t.isGroupAddress { + err = cerrs.New("", 1, "") + } + return nil, err +} diff --git a/x/marker/keeper/keeper.go b/x/marker/keeper/keeper.go index 3b1b633d6b..1ba741baab 100644 --- a/x/marker/keeper/keeper.go +++ b/x/marker/keeper/keeper.go @@ -86,6 +86,9 @@ type Keeper struct { // When sending from one of these, if there are required attributes, the destination must have them; // if there aren't required attributes, it behaves as if the sender has transfer permission. reqAttrBypassAddrs types.ImmutableAccAddresses + + // groupChecker provides a way to check if an account is in a group. + groupChecker types.GroupChecker } // NewKeeper returns a marker keeper. It handles: @@ -105,6 +108,7 @@ func NewKeeper( nameKeeper types.NameKeeper, ibcTransferServer types.IbcTransferMsgServer, reqAttrBypassAddrs []sdk.AccAddress, + checker types.GroupChecker, ) Keeper { if !paramSpace.HasKeyTable() { paramSpace = paramSpace.WithKeyTable(types.ParamKeyTable()) @@ -125,6 +129,7 @@ func NewKeeper( ibcTransferModuleAddr: authtypes.NewModuleAddress(ibctypes.ModuleName), ibcTransferServer: ibcTransferServer, reqAttrBypassAddrs: types.NewImmutableAccAddresses(reqAttrBypassAddrs), + groupChecker: checker, } bankKeeper.AppendSendRestriction(rv.SendRestrictionFn) return rv diff --git a/x/marker/keeper/keeper_test.go b/x/marker/keeper/keeper_test.go index 9573a388a9..986ccd0576 100644 --- a/x/marker/keeper/keeper_test.go +++ b/x/marker/keeper/keeper_test.go @@ -18,6 +18,7 @@ import ( distrtypes "github.com/cosmos/cosmos-sdk/x/distribution/types" "github.com/cosmos/cosmos-sdk/x/feegrant" govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" + "github.com/cosmos/cosmos-sdk/x/group" paramtypes "github.com/cosmos/cosmos-sdk/x/params/types" "github.com/cosmos/cosmos-sdk/x/quarantine" stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" @@ -754,9 +755,28 @@ func TestCanForceTransferFrom(t *testing.T) { app.AccountKeeper.SetAccount(ctx, acc) } + createGroup := func() sdk.AccAddress { + goCtx := sdk.WrapSDKContext(ctx) + msg, err := group.NewMsgCreateGroupWithPolicy("cosmos10d07y265gmmuvt4z0w9aw880jnsr700j6zn9kn", + []group.MemberRequest{ + { + Address: "cosmos10d07y265gmmuvt4z0w9aw880jnsr700j6zn9kn", + Weight: "1", + Metadata: "", + }, + }, + "", "", true, group.NewPercentageDecisionPolicy("0.5", time.Second, time.Second)) + require.NoError(t, err, "NewMsgCreateGroupWithPolicy") + res, err := app.GroupKeeper.CreateGroupWithPolicy(goCtx, msg) + require.NoError(t, err, "CreateGroupWithPolicy") + + return sdk.MustAccAddressFromBech32(res.GroupPolicyAddress) + } + addrNoAcc := sdk.AccAddress("addrNoAcc___________") addrSeq0 := sdk.AccAddress("addrSeq0____________") addrSeq1 := sdk.AccAddress("addrSeq1____________") + addrGroup := createGroup() setAcc(addrSeq0, 0) setAcc(addrSeq1, 1) @@ -768,6 +788,7 @@ func TestCanForceTransferFrom(t *testing.T) { {name: "address without an account", from: addrNoAcc, exp: true}, {name: "address with sequence 0", from: addrSeq0, exp: false}, {name: "address with sequence 1", from: addrSeq1, exp: true}, + {name: "group address", from: addrGroup, exp: true}, } for _, tc := range tests { @@ -1831,7 +1852,7 @@ func TestBypassAddrsLocked(t *testing.T) { sdk.AccAddress("addrs[4]____________"), } - mk := markerkeeper.NewKeeper(nil, nil, paramtypes.NewSubspace(nil, nil, nil, nil, "test"), nil, &dummyBankKeeper{}, nil, nil, nil, nil, nil, addrs) + mk := markerkeeper.NewKeeper(nil, nil, paramtypes.NewSubspace(nil, nil, nil, nil, "test"), nil, &dummyBankKeeper{}, nil, nil, nil, nil, nil, addrs, nil) // Now that the keeper has been created using the provided addresses, change the first byte of // the first address to something else. Then, get the addresses back from the keeper and make diff --git a/x/marker/keeper/marker.go b/x/marker/keeper/marker.go index 7893e50a77..7b14970ab9 100644 --- a/x/marker/keeper/marker.go +++ b/x/marker/keeper/marker.go @@ -680,6 +680,11 @@ func (k Keeper) TransferCoin(ctx sdk.Context, from, to, admin sdk.AccAddress, am // canForceTransferFrom returns true if funds can be forcefully transferred out of the provided address. func (k Keeper) canForceTransferFrom(ctx sdk.Context, from sdk.AccAddress) bool { acc := k.authKeeper.GetAccount(ctx, from) + // If the account is a group address, then it will allow the transfer. + if k.groupChecker != nil && k.groupChecker.IsGroupAddress(ctx, from) { + return true + } + // If acc is nil, there's no funds in it, so the transfer will fail anyway. // In that case, return true from here so it can fail later with a more accurate message. // If there is an account, only allow force transfers if the sequence number isn't zero. diff --git a/x/marker/keeper/proposal_handler_test.go b/x/marker/keeper/proposal_handler_test.go index a6a62a4869..29f34ae514 100644 --- a/x/marker/keeper/proposal_handler_test.go +++ b/x/marker/keeper/proposal_handler_test.go @@ -34,7 +34,7 @@ type IntegrationTestSuite struct { func (s *IntegrationTestSuite) SetupSuite() { s.app = provenance.Setup(s.T()) s.ctx = s.app.BaseApp.NewContext(false, tmproto.Header{}) - s.k = markerkeeper.NewKeeper(s.app.AppCodec(), s.app.GetKey(markertypes.ModuleName), s.app.GetSubspace(markertypes.ModuleName), s.app.AccountKeeper, s.app.BankKeeper, s.app.AuthzKeeper, s.app.FeeGrantKeeper, s.app.AttributeKeeper, s.app.NameKeeper, s.app.TransferKeeper, nil) + s.k = markerkeeper.NewKeeper(s.app.AppCodec(), s.app.GetKey(markertypes.ModuleName), s.app.GetSubspace(markertypes.ModuleName), s.app.AccountKeeper, s.app.BankKeeper, s.app.AuthzKeeper, s.app.FeeGrantKeeper, s.app.AttributeKeeper, s.app.NameKeeper, s.app.TransferKeeper, nil, nil) s.accountAddr = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()) } diff --git a/x/marker/simulation/proposals_test.go b/x/marker/simulation/proposals_test.go index ecca361ebd..c89972f1e3 100644 --- a/x/marker/simulation/proposals_test.go +++ b/x/marker/simulation/proposals_test.go @@ -43,6 +43,7 @@ func TestProposalContents(t *testing.T) { app.NameKeeper, app.TransferKeeper, nil, + nil, ), ) require.Len(t, weightedProposalContent, 6) diff --git a/x/marker/types/expected_keepers.go b/x/marker/types/expected_keepers.go index 3326933cd1..b9f2a07eec 100644 --- a/x/marker/types/expected_keepers.go +++ b/x/marker/types/expected_keepers.go @@ -97,3 +97,8 @@ type GovKeeper interface { type IbcTransferMsgServer interface { Transfer(goCtx context.Context, msg *transfertypes.MsgTransfer) (*transfertypes.MsgTransferResponse, error) } + +// GroupChecker defines the functionality for checking if an account is part of a group. +type GroupChecker interface { + IsGroupAddress(sdk.Context, sdk.AccAddress) bool +}