Skip to content

Commit

Permalink
Allow feegrants from marker accounts again. (#2111)
Browse files Browse the repository at this point in the history
* Write a unit test (that currently fails) that attempts to use a feegrant for a MsgSend.

* In the antehandler stuff, if a feegrant is looked for and used, make note of that in the context where we're collecting fees. Then, in the marker send restriction, if coming from a marker, check for that feegrant flag to bypass checking that there's a transfer agent with withdraw permissions.

* Unit tests on the new context helpers.

* Add some test cases to the SendRestrictionFn tests to hit the scenario where there's a feegrant in use.

* Put a better comment on the feegrant bypass.

* Fix imports in the send_restrictions.

* Delete an unused constant: AddressHasAccessKey = "address_has_access"

* Final touches on that updated comment.

* Add changelog entry.

* Update the flowchart that covers this bug.

* Remove the entry from CHANGELOG.md and add a new unreleased entry (the new way).
  • Loading branch information
SpicyLemon committed Aug 15, 2024
1 parent 247be8e commit 954a6d6
Show file tree
Hide file tree
Showing 9 changed files with 253 additions and 22 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* Allow marker funds to be used via feegrant again [#2110](https://github.com/provenance-io/provenance/issues/2110).
19 changes: 13 additions & 6 deletions internal/antewrapper/provenance_fee.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"github.com/cosmos/cosmos-sdk/x/auth/types"
bankkeeper "github.com/cosmos/cosmos-sdk/x/bank/keeper"

internalsdk "github.com/provenance-io/provenance/internal/sdk"
msgfeestypes "github.com/provenance-io/provenance/x/msgfees/types"
)

Expand Down Expand Up @@ -91,7 +92,7 @@ func (dfd ProvenanceDeductFeeDecorator) checkDeductBaseFee(ctx sdk.Context, feeT
return sdkerrors.ErrInvalidRequest.Wrap(err.Error())
}

deductFeesFrom, err := GetFeePayerUsingFeeGrant(ctx, dfd.feegrantKeeper, feeTx, baseFeeToConsume, msgs)
deductFeesFrom, usedFeeGrant, err := GetFeePayerUsingFeeGrant(ctx, dfd.feegrantKeeper, feeTx, baseFeeToConsume, msgs)
if err != nil {
return err
}
Expand Down Expand Up @@ -130,7 +131,11 @@ func (dfd ProvenanceDeductFeeDecorator) checkDeductBaseFee(ctx sdk.Context, feeT
// We don't do this when simulating since we're simulating.
// And we don't do this during InitGenesis since those Txs don't have any fees on them at all.
if !simulate && !IsInitGenesis(ctx) && !baseFeeToConsume.IsZero() {
err = DeductFees(dfd.bankKeeper, ctx, deductFeesFrom, baseFeeToConsume)
dctx := ctx
if usedFeeGrant {
dctx = internalsdk.WithFeeGrantInUse(ctx)
}
err = DeductFees(dfd.bankKeeper, dctx, deductFeesFrom, baseFeeToConsume)
if err != nil {
return err
}
Expand All @@ -151,29 +156,31 @@ func (dfd ProvenanceDeductFeeDecorator) checkDeductBaseFee(ctx sdk.Context, feeT
return nil
}

func GetFeePayerUsingFeeGrant(ctx sdk.Context, feegrantKeeper msgfeestypes.FeegrantKeeper, feeTx sdk.FeeTx, fee sdk.Coins, msgs []sdk.Msg) (sdk.AccAddress, error) {
func GetFeePayerUsingFeeGrant(ctx sdk.Context, feegrantKeeper msgfeestypes.FeegrantKeeper, feeTx sdk.FeeTx, fee sdk.Coins, msgs []sdk.Msg) (sdk.AccAddress, bool, error) {
feePayer := sdk.AccAddress(feeTx.FeePayer())
feeGranter := sdk.AccAddress(feeTx.FeeGranter())
deductFeesFrom := feePayer
usedFeeGrant := false

// if feegranter set deduct base fee from feegranter account.
// this works with only when feegrant enabled.
if feeGranter != nil && !bytes.Equal(feeGranter, feePayer) {
if feegrantKeeper == nil {
return nil, sdkerrors.ErrInvalidRequest.Wrap("fee grants are not enabled")
return nil, false, sdkerrors.ErrInvalidRequest.Wrap("fee grants are not enabled")
}
err := feegrantKeeper.UseGrantedFees(ctx, feeGranter, feePayer, fee, msgs)
if err != nil {
msgTypes := make([]string, len(msgs))
for i, msg := range msgs {
msgTypes[i] = sdk.MsgTypeURL(msg)
}
return nil, cerrs.Wrapf(err, "failed to use fee grant: granter: %s, grantee: %s, fee: %q, msgs: %q", feeGranter, feePayer, fee, msgTypes)
return nil, false, cerrs.Wrapf(err, "failed to use fee grant: granter: %s, grantee: %s, fee: %q, msgs: %q", feeGranter, feePayer, fee, msgTypes)
}
deductFeesFrom = feeGranter
usedFeeGrant = true
}

return deductFeesFrom, nil
return deductFeesFrom, usedFeeGrant, nil
}

// CalculateBaseFee calculates the base fee.
Expand Down
6 changes: 5 additions & 1 deletion internal/handlers/msg_fee_invoker.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
bankkeeper "github.com/cosmos/cosmos-sdk/x/bank/keeper"

"github.com/provenance-io/provenance/internal/antewrapper"
internalsdk "github.com/provenance-io/provenance/internal/sdk"
msgfeestypes "github.com/provenance-io/provenance/x/msgfees/types"
)

Expand Down Expand Up @@ -69,7 +70,7 @@ func (afd MsgFeeInvoker) Invoke(ctx sdk.Context, _ bool) (sdk.Coins, sdk.Events,
baseFeeConsumed := feeGasMeter.BaseFeeConsumed()
unchargedFees, _ := feeTx.GetFee().SafeSub(baseFeeConsumed...)

deductFeesFrom, err := antewrapper.GetFeePayerUsingFeeGrant(ctx, afd.feegrantKeeper, feeTx, unchargedFees, tx.GetMsgs())
deductFeesFrom, usedFeegrant, err := antewrapper.GetFeePayerUsingFeeGrant(ctx, afd.feegrantKeeper, feeTx, unchargedFees, tx.GetMsgs())
if err != nil {
return nil, nil, err
}
Expand All @@ -82,6 +83,9 @@ func (afd MsgFeeInvoker) Invoke(ctx sdk.Context, _ bool) (sdk.Coins, sdk.Events,
// If there's fees left to collect, or there were consumed fees, deduct/distribute them now.
if !unchargedFees.IsZero() || !consumedFees.IsZero() {
eventCtx := ctx.WithEventManager(sdk.NewEventManager())
if usedFeegrant {
eventCtx = internalsdk.WithFeeGrantInUse(eventCtx)
}
err = afd.msgFeeKeeper.DeductFeesDistributions(afd.bankKeeper, eventCtx, deductFeesFromAcc, unchargedFees, feeGasMeter.FeeConsumedDistributions())
if err != nil {
return nil, nil, err
Expand Down
36 changes: 36 additions & 0 deletions internal/sdk/context.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
package sdk

import (
"context"

sdk "github.com/cosmos/cosmos-sdk/types"
)

var (
feeGranteeKey = "pio-feegrant-in-use"
)

// WithFeeGrantInUse returns a new context that will indicate that a feegrant is being used.
func WithFeeGrantInUse[C context.Context](ctx C) C {
sdkCtx := sdk.UnwrapSDKContext(ctx)
sdkCtx = sdkCtx.WithValue(feeGranteeKey, true)
return context.Context(sdkCtx).(C)
}

// WithoutFeeGrantInUse returns a new context that will indicate that a feegrant is NOT being used.
func WithoutFeeGrantInUse[C context.Context](ctx C) C {
sdkCtx := sdk.UnwrapSDKContext(ctx)
sdkCtx = sdkCtx.WithValue(feeGranteeKey, false)
return context.Context(sdkCtx).(C)
}

// HasFeeGrantInUse checks the context to see if the a feegrant is being used.
func HasFeeGrantInUse[C context.Context](ctx C) bool {
sdkCtx := sdk.UnwrapSDKContext(ctx)
bypassValue := sdkCtx.Value(feeGranteeKey)
if bypassValue == nil {
return false
}
bypass, isBool := bypassValue.(bool)
return isBool && bypass
}
73 changes: 73 additions & 0 deletions internal/sdk/context_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
package sdk

import (
"testing"

cmtproto "github.com/cometbft/cometbft/proto/tendermint/types"
"github.com/stretchr/testify/assert"

sdk "github.com/cosmos/cosmos-sdk/types"
)

func TestFeeGrantContextFuncs(t *testing.T) {
tests := []struct {
name string
ctx sdk.Context
exp bool
}{
{
name: "brand new mostly empty context",
ctx: sdk.NewContext(nil, cmtproto.Header{}, false, nil),
exp: false,
},
{
name: "context with fee grant in use",
ctx: WithFeeGrantInUse(sdk.NewContext(nil, cmtproto.Header{}, false, nil)),
exp: true,
},
{
name: "context with fee grant in use on one that originally was without it",
ctx: WithFeeGrantInUse(WithoutFeeGrantInUse(sdk.NewContext(nil, cmtproto.Header{}, false, nil))),
exp: true,
},
{
name: "context with fee grant in use twice",
ctx: WithFeeGrantInUse(WithFeeGrantInUse(sdk.NewContext(nil, cmtproto.Header{}, false, nil))),
exp: true,
},
{
name: "context without fee grant in use",
ctx: WithoutFeeGrantInUse(sdk.NewContext(nil, cmtproto.Header{}, false, nil)),
exp: false,
},
{
name: "context without fee grant in use on one that originally had it",
ctx: WithoutFeeGrantInUse(WithFeeGrantInUse(sdk.NewContext(nil, cmtproto.Header{}, false, nil))),
exp: false,
},
{
name: "context without fee grant in use twice",
ctx: WithoutFeeGrantInUse(WithoutFeeGrantInUse(sdk.NewContext(nil, cmtproto.Header{}, false, nil))),
exp: false,
},
}

for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
actual := HasFeeGrantInUse(tc.ctx)
assert.Equal(t, tc.exp, actual, "HasFeeGrantInUse")
})
}
}

func TestFeeGrantInUseFuncsDoNotModifyProvided(t *testing.T) {
origCtx := sdk.NewContext(nil, cmtproto.Header{}, false, nil)
assert.False(t, HasFeeGrantInUse(origCtx), "HasFeeGrantInUse(origCtx)")
afterWith := WithFeeGrantInUse(origCtx)
assert.True(t, HasFeeGrantInUse(afterWith), "HasFeeGrantInUse(afterWith)")
assert.False(t, HasFeeGrantInUse(origCtx), "HasFeeGrantInUse(origCtx) after giving it to WithFeeGrantInUse")
afterWithout := WithoutFeeGrantInUse(afterWith)
assert.False(t, HasFeeGrantInUse(afterWithout), "HasFeeGrantInUse(afterWithout)")
assert.True(t, HasFeeGrantInUse(afterWith), "HasFeeGrantInUse(afterWith) after giving it to WithoutFeeGrantInUse")
assert.False(t, HasFeeGrantInUse(origCtx), "HasFeeGrantInUse(origCtx) after giving afterWith to WithoutFeeGrantInUse")
}
62 changes: 62 additions & 0 deletions x/marker/client/cli/cli_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
testnet "github.com/cosmos/cosmos-sdk/testutil/network"
sdk "github.com/cosmos/cosmos-sdk/types"
authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
bankcli "github.com/cosmos/cosmos-sdk/x/bank/client/cli"
banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
govcli "github.com/cosmos/cosmos-sdk/x/gov/client/cli"
govtypes "github.com/cosmos/cosmos-sdk/x/gov/types"
Expand Down Expand Up @@ -124,6 +125,7 @@ func (s *IntegrationTestSuite) SetupSuite() {
bal(markertypes.MustGetMarkerAddress("lockedcoin"), coin(1000, "lockedcoin")),
bal(markertypes.MustGetMarkerAddress("propcoin"), coin(1000, "propcoin")),
bal(markertypes.MustGetMarkerAddress("authzhotdog"), coin(800, "authzhotdog")),
bal(markertypes.MustGetMarkerAddress("grantcoin"), coin(5000, s.cfg.BondDenom)),
)

return bankGenState
Expand Down Expand Up @@ -227,6 +229,25 @@ func (s *IntegrationTestSuite) SetupSuite() {
Denom: s.holderDenom,
AllowForcedTransfer: true,
},
{
BaseAccount: &authtypes.BaseAccount{
Address: markertypes.MustGetMarkerAddress("grantcoin").String(),
AccountNumber: 160,
Sequence: 0,
},
Status: markertypes.StatusActive,
SupplyFixed: false,
MarkerType: markertypes.MarkerType_Coin,
AllowGovernanceControl: true,
Supply: sdkmath.NewInt(0),
Denom: "grantcoin",
AllowForcedTransfer: false,
AccessControl: []markertypes.AccessGrant{
*markertypes.NewAccessGrant(s.accountAddresses[0], []markertypes.Access{
markertypes.Access_Admin, markertypes.Access_Deposit, markertypes.Access_Withdraw,
}),
},
},
}
markerData.Markers = append(markerData.Markers, newMarkers...)

Expand Down Expand Up @@ -2600,3 +2621,44 @@ func (s *IntegrationTestSuite) TestUpdateMarkerParamsCmd() {
})
}
}

func (s *IntegrationTestSuite) TestPayingWithFeegrant() {
curClientCtx := s.testnet.Validators[0].ClientCtx
defer func() {
s.testnet.Validators[0].ClientCtx = curClientCtx
}()
s.testnet.Validators[0].ClientCtx = s.testnet.Validators[0].ClientCtx.WithKeyring(s.keyring)

setupOK := s.Run("Create a feegrant with the marker as the granter", func() {
cmd := markercli.GetCmdFeeGrant()
args := []string{
"grantcoin",
s.accountAddresses[0].String(),
s.accountAddresses[2].String(),
fmt.Sprintf("--%s=%s", markercli.FlagSpendLimit, sdk.NewInt64Coin(s.cfg.BondDenom, 5000)),
fmt.Sprintf("--%s=%s", markercli.FlagExpiration, getFormattedExpiration(oneYear)),
fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation),
fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastSync),
fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewInt64Coin(s.cfg.BondDenom, 10)).String()),
}
testcli.NewTxExecutor(cmd, args).Execute(s.T(), s.testnet)
})

s.Run("Attempt to use the feegrant for a send", func() {
if !setupOK {
s.T().Skipf("Skipping due to setup failure.")
}
addrCdc := s.cfg.Codec.InterfaceRegistry().SigningContext().AddressCodec()
cmd := bankcli.NewSendTxCmd(addrCdc)
args := []string{
s.accountAddresses[2].String(),
s.accountAddresses[1].String(),
sdk.NewInt64Coin(s.cfg.BondDenom, 17).String(),
"--" + flags.FlagFees, sdk.NewCoins(sdk.NewInt64Coin(s.cfg.BondDenom, 10)).String(),
"--" + flags.FlagBroadcastMode, flags.BroadcastSync,
"--" + flags.FlagSkipConfirmation,
"--fee-granter", markertypes.MustGetMarkerAddress("grantcoin").String(),
}
testcli.NewTxExecutor(cmd, args).Execute(s.T(), s.testnet)
})
}
29 changes: 17 additions & 12 deletions x/marker/keeper/send_restrictions.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,11 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"
banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"

internalsdk "github.com/provenance-io/provenance/internal/sdk"
attrTypes "github.com/provenance-io/provenance/x/attribute/types"
"github.com/provenance-io/provenance/x/marker/types"
)

const AddressHasAccessKey = "address_has_access"

var _ banktypes.SendRestrictionFn = Keeper{}.SendRestrictionFn

func (k Keeper) SendRestrictionFn(goCtx context.Context, fromAddr, toAddr sdk.AccAddress, amt sdk.Coins) (sdk.AccAddress, error) {
Expand All @@ -40,17 +39,23 @@ func (k Keeper) SendRestrictionFn(goCtx context.Context, fromAddr, toAddr sdk.Ac
// If it's coming from a marker, make sure the withdraw is allowed.
admin := types.GetTransferAgent(ctx)
if fromMarker, _ := k.GetMarker(ctx, fromAddr); fromMarker != nil {
// It shouldn't be possible for the fromAddr to be a marker without a transfer
// agent because no keys exist for any marker accounts (so it can't sign a Tx).
// So, to be on the safe side, we return an error if that's the case.
if len(admin) == 0 {
return nil, fmt.Errorf("cannot withdraw from marker account %s (%s)",
fromAddr.String(), fromMarker.GetDenom())
}
// The only ways to legitimately send from a marker account is to have a transfer agent with
// withdraw permissions, or through a feegrant. The only way to have a feegrant from
// a marker account is if an admin creates one using the marker module's GrantAllowance endpoint.
// So if a feegrant is in use, that means the sending of these funds is authorized.
// We also assume that other stuff is handling the actual checking and use of that feegrant,
// so we don't need to worry about its details in here, and that HasFeeGrantInUse is only ever
// true when collecting fees.
if !internalsdk.HasFeeGrantInUse(ctx) {
if len(admin) == 0 {
return nil, fmt.Errorf("cannot withdraw from marker account %s (%s)",
fromAddr.String(), fromMarker.GetDenom())
}

// That transfer agent must have withdraw access on the marker we're taking from.
if err := fromMarker.ValidateAddressHasAccess(admin, types.Access_Withdraw); err != nil {
return nil, err
// That transfer agent must have withdraw access on the marker we're taking from.
if err := fromMarker.ValidateAddressHasAccess(admin, types.Access_Withdraw); err != nil {
return nil, err
}
}

// Check to see if marker is active; the coins created by a marker can only be withdrawn when it is active.
Expand Down
Loading

0 comments on commit 954a6d6

Please sign in to comment.