Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

R4R: Multimsg ante handler Jae's proposal #3787

Closed
wants to merge 10 commits into from
Closed

R4R: Multimsg ante handler Jae's proposal #3787

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c775602
Jae's proposal
sunnya97 Mar 2, 2019
ffbda5c
Remove SendEnabled
jaekwon Mar 3, 2019
2907ef1
fix lcd test
jaekwon Mar 3, 2019
10af5c2
fix bug
jaekwon Mar 4, 2019
60890da
atomsToUatoms
sunnya97 Mar 4, 2019
ddbe903
Update cmd/gaia/app/app.go
alexanderbez Mar 4, 2019
e27b979
Update cmd/gaia/app/app.go
sunnya97 Mar 4, 2019
c15322c
Update cmd/gaia/app/app.go
sunnya97 Mar 4, 2019
d2e9c5f
cli tests run w/ send_enabled gotag
jaekwon Mar 5, 2019
5255fa6
fix comment
jaekwon Mar 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 50 additions & 1 deletion cmd/gaia/app/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ const (
appName = "GaiaApp"
// DefaultKeyPass contains the default key password for genesis transactions
DefaultKeyPass = "12345678"

atomsToUatoms = 1000000
)

// default home directories for expected binaries
Expand Down Expand Up @@ -170,7 +172,30 @@ func NewGaiaApp(logger log.Logger, db dbm.DB, traceStore io.Writer, loadLatest b
)
app.SetInitChainer(app.initChainer)
app.SetBeginBlocker(app.BeginBlocker)
app.SetAnteHandler(auth.NewAnteHandler(app.accountKeeper, app.feeCollectionKeeper))

authAnteHandler := auth.NewAnteHandler(app.accountKeeper, app.feeCollectionKeeper)

filterAnteHandler := func(ctx sdk.Context, tx sdk.Tx, simulate bool) (newCtx sdk.Context, result sdk.Result, abort bool) {
Copy link
Contributor

@alexanderbez alexanderbez Mar 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Hmmm maybe I'm missing something glaringly obvious, but why must we have a "filterAnteHandler" in app? Why can't we just use the regular ante handler and have the sendEnabled enabled check? Then we won't need any hacky build stuff for this. Yes, the ante handler would have to be provided a "keeper contract" that implements GetSendEnabled.

What if a bunch of nodes choose to run without this build setup? What if this introduces a spam or DoS vector? (since the entire ante handler is executed first)

Copy link
Contributor

@alexanderbez alexanderbez Mar 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Ohhh because it needs access to x/bank for message validation....sigh. I still think we can avoid this though. NewAnteHandler can be passed a tx validator function (which is defined in bank, e.g. func(tx sdk.Tx) sdk.Result) . Then we dont need any filter ante handler OR hacky build steps.

Copy link
Member Author

Choose a reason for hiding this comment

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

Because other people are using the bank module rn, and we don't want to add Hub specific functionality to the module. We wanted to isolate this functionality change to just the gaia package.

Really, the proper way to do this is to allow for rerouting subroutes. So the route of the bank module msgs would be:
bank/send
bank/multisend

And then in the router, we route
bank/* to the bank module, but bank/multisend to a special handler defined in the gaia folder.

However, we don't have time to do this rn, as this would involve changes to the router functionality.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess we could just temporarily fork the entire bank module and add it as a special bank module in the gaia folder. And then to enable transfers point it back to the SDK bank module. That's actually not a bad idea.....

@cwgoes @jaekwon @zmanian

Copy link
Contributor

@alexanderbez alexanderbez Mar 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah anything to avoid involving the build pipeline would be an ideal solution. Seems trivial to do @sunnya97

Copy link
Member

Choose a reason for hiding this comment

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

I have all of these same concerns about using the build pipeline. I think, while messy, that forking the bank module may be the best way to do this. Looking forward to continuing this discussion in the sdk meeting today.

Copy link
Member

Choose a reason for hiding this comment

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

Also this will be a very easy change to revert if we do that (just reimport the normal bank module into app.go)

Copy link
Contributor

Choose a reason for hiding this comment

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

we don't know how messy it will be, this is done, lets go with this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternative as suggested by @sunnya97: #3807

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not messy at all. I really don't think we should rely on the build process to enforce business logic (especially since it ties into consensus).

newCtx, result, abort = authAnteHandler(ctx, tx, simulate)
if abort == true {
return
}

for _, msg := range tx.GetMsgs() {
switch msg.(type) {
case bank.MsgSend:
// LCD tests requires this to be commented out
// return ctx, bank.ErrSendDisabled(bank.DefaultCodespace).Result(), true
Copy link
Contributor

Choose a reason for hiding this comment

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

Why? We should modify those tests, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem safe, now we'll allow sends (?)

Copy link
Member Author

@sunnya97 sunnya97 Mar 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Can I use the Makefile to set an environment variable or something that I can read from in the code? Is that safe?

Copy link
Contributor

Choose a reason for hiding this comment

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

we can't use an env variable but we can use a build flag, i'll do this.

case bank.MsgMultiSend:
if !CheckTransferDisabledBurnMultiSend(msg.(bank.MsgMultiSend)) {
return ctx, bank.ErrSendDisabled(bank.DefaultCodespace).Result(), true
}
}
}
return
}

app.SetAnteHandler(filterAnteHandler)
app.SetEndBlocker(app.EndBlocker)

if loadLatest {
Expand All @@ -183,6 +208,30 @@ func NewGaiaApp(logger log.Logger, db dbm.DB, traceStore io.Writer, loadLatest b
return app
}

// CheckTransferDisabledBurnMultiSend
func validateMultiSendTransfersDisabled(msg bank.MsgMultiSend) bool {
nineAtoms := sdk.Coins{sdk.NewInt64Coin("uatom", 9*atomsToUatoms)}
oneAtom := sdk.Coins{sdk.NewInt64Coin("uatom", 10*atomsToUatoms)}

if len(msg.Inputs) != 1 {
return false
}
if len(msg.Outputs) != 2 {
return false
}

if !msg.Outputs[0].Address.Equals(bank.BurnedCoinsAccAddr) {
return false
}
if !msg.Outputs[0].Coins.IsEqual(nineAtoms) {
return false
}
if !msg.Outputs[1].Coins.IsEqual(oneAtom) {
return false
}
return true
}

// custom tx codec
func MakeCodec() *codec.Codec {
var cdc = codec.New()
Expand Down
2 changes: 1 addition & 1 deletion cmd/gaia/app/sim_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ func appStateRandomizedFn(r *rand.Rand, accs []simulation.Account, genesisTimest
}
fmt.Printf("Selected randomly generated auth parameters:\n\t%+v\n", authGenesis)

bankGenesis := bank.NewGenesisState(r.Int63n(2) == 0)
bankGenesis := bank.NewGenesisState()
fmt.Printf("Selected randomly generated bank parameters:\n\t%+v\n", bankGenesis)

// Random genesis states
Expand Down
10 changes: 4 additions & 6 deletions x/bank/genesis.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,25 +6,23 @@ import (

// GenesisState is the bank state that must be provided at genesis.
type GenesisState struct {
SendEnabled bool `json:"send_enabled"`
}

// NewGenesisState creates a new genesis state.
func NewGenesisState(sendEnabled bool) GenesisState {
return GenesisState{SendEnabled: sendEnabled}
func NewGenesisState() GenesisState {
return GenesisState{}
}

// DefaultGenesisState returns a default genesis state
func DefaultGenesisState() GenesisState { return NewGenesisState(true) }
func DefaultGenesisState() GenesisState { return NewGenesisState() }

// InitGenesis sets distribution information for genesis.
func InitGenesis(ctx sdk.Context, keeper Keeper, data GenesisState) {
keeper.SetSendEnabled(ctx, data.SendEnabled)
}

// ExportGenesis returns a GenesisState for a given context and keeper.
func ExportGenesis(ctx sdk.Context, keeper Keeper) GenesisState {
return NewGenesisState(keeper.GetSendEnabled(ctx))
return NewGenesisState()
}

// ValidateGenesis performs basic validation of bank genesis data returning an
Expand Down
6 changes: 0 additions & 6 deletions x/bank/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,6 @@ func NewHandler(k Keeper) sdk.Handler {

// Handle MsgSend.
func handleMsgSend(ctx sdk.Context, k Keeper, msg MsgSend) sdk.Result {
if !k.GetSendEnabled(ctx) {
return ErrSendDisabled(k.Codespace()).Result()
}
tags, err := k.SendCoins(ctx, msg.FromAddress, msg.ToAddress, msg.Amount)
if err != nil {
return err.Result()
Expand All @@ -37,9 +34,6 @@ func handleMsgSend(ctx sdk.Context, k Keeper, msg MsgSend) sdk.Result {
// Handle MsgMultiSend.
func handleMsgMultiSend(ctx sdk.Context, k Keeper, msg MsgMultiSend) sdk.Result {
// NOTE: totalIn == totalOut should already have been checked
if !k.GetSendEnabled(ctx) {
return ErrSendDisabled(k.Codespace()).Result()
}
tags, err := k.InputOutputCoins(ctx, msg.Inputs, msg.Outputs)
if err != nil {
return err.Result()
Expand Down
20 changes: 4 additions & 16 deletions x/bank/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,14 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/x/auth"
"github.com/cosmos/cosmos-sdk/x/params"
"github.com/tendermint/tendermint/crypto"
)

var _ Keeper = (*BaseKeeper)(nil)

// Account to send burned coins to
var BurnedCoinsAccAddr = sdk.AccAddress(crypto.AddressHash([]byte("bankBurnedCoins")))

// Keeper defines a module interface that facilitates the transfer of coins
// between accounts.
type Keeper interface {
Expand Down Expand Up @@ -120,9 +124,6 @@ type SendKeeper interface {
ViewKeeper

SendCoins(ctx sdk.Context, fromAddr sdk.AccAddress, toAddr sdk.AccAddress, amt sdk.Coins) (sdk.Tags, sdk.Error)

GetSendEnabled(ctx sdk.Context) bool
SetSendEnabled(ctx sdk.Context, enabled bool)
}

var _ SendKeeper = (*BaseSendKeeper)(nil)
Expand Down Expand Up @@ -158,19 +159,6 @@ func (keeper BaseSendKeeper) SendCoins(
return sendCoins(ctx, keeper.ak, fromAddr, toAddr, amt)
}

// GetSendEnabled returns the current SendEnabled
// nolint: errcheck
func (keeper BaseSendKeeper) GetSendEnabled(ctx sdk.Context) bool {
var enabled bool
keeper.paramSpace.Get(ctx, ParamStoreKeySendEnabled, &enabled)
return enabled
}

// SetSendEnabled sets the send enabled
func (keeper BaseSendKeeper) SetSendEnabled(ctx sdk.Context, enabled bool) {
keeper.paramSpace.Set(ctx, ParamStoreKeySendEnabled, &enabled)
}

var _ ViewKeeper = (*BaseViewKeeper)(nil)

// ViewKeeper defines a module interface that facilitates read only access to
Expand Down
7 changes: 0 additions & 7 deletions x/bank/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ func TestKeeper(t *testing.T) {
input := setupTestInput()
ctx := input.ctx
bankKeeper := NewBaseKeeper(input.ak, input.pk.Subspace(DefaultParamspace), DefaultCodespace)
bankKeeper.SetSendEnabled(ctx, true)

addr := sdk.AccAddress([]byte("addr1"))
addr2 := sdk.AccAddress([]byte("addr2"))
Expand Down Expand Up @@ -139,7 +138,6 @@ func TestSendKeeper(t *testing.T) {
paramSpace := input.pk.Subspace(DefaultParamspace)
bankKeeper := NewBaseKeeper(input.ak, paramSpace, DefaultCodespace)
sendKeeper := NewBaseSendKeeper(input.ak, paramSpace, DefaultCodespace)
bankKeeper.SetSendEnabled(ctx, true)

addr := sdk.AccAddress([]byte("addr1"))
addr2 := sdk.AccAddress([]byte("addr2"))
Expand Down Expand Up @@ -187,7 +185,6 @@ func TestViewKeeper(t *testing.T) {
ctx := input.ctx
paramSpace := input.pk.Subspace(DefaultParamspace)
bankKeeper := NewBaseKeeper(input.ak, paramSpace, DefaultCodespace)
bankKeeper.SetSendEnabled(ctx, true)
viewKeeper := NewBaseViewKeeper(input.ak, DefaultCodespace)

addr := sdk.AccAddress([]byte("addr1"))
Expand Down Expand Up @@ -216,7 +213,6 @@ func TestVestingAccountSend(t *testing.T) {
origCoins := sdk.Coins{sdk.NewInt64Coin("steak", 100)}
sendCoins := sdk.Coins{sdk.NewInt64Coin("steak", 50)}
bankKeeper := NewBaseKeeper(input.ak, input.pk.Subspace(DefaultParamspace), DefaultCodespace)
bankKeeper.SetSendEnabled(ctx, true)

addr1 := sdk.AccAddress([]byte("addr1"))
addr2 := sdk.AccAddress([]byte("addr2"))
Expand Down Expand Up @@ -250,7 +246,6 @@ func TestVestingAccountReceive(t *testing.T) {
origCoins := sdk.Coins{sdk.NewInt64Coin("steak", 100)}
sendCoins := sdk.Coins{sdk.NewInt64Coin("steak", 50)}
bankKeeper := NewBaseKeeper(input.ak, input.pk.Subspace(DefaultParamspace), DefaultCodespace)
bankKeeper.SetSendEnabled(ctx, true)

addr1 := sdk.AccAddress([]byte("addr1"))
addr2 := sdk.AccAddress([]byte("addr2"))
Expand Down Expand Up @@ -284,7 +279,6 @@ func TestDelegateCoins(t *testing.T) {
origCoins := sdk.Coins{sdk.NewInt64Coin("steak", 100)}
delCoins := sdk.Coins{sdk.NewInt64Coin("steak", 50)}
bankKeeper := NewBaseKeeper(input.ak, input.pk.Subspace(DefaultParamspace), DefaultCodespace)
bankKeeper.SetSendEnabled(ctx, true)

addr1 := sdk.AccAddress([]byte("addr1"))
addr2 := sdk.AccAddress([]byte("addr2"))
Expand Down Expand Up @@ -321,7 +315,6 @@ func TestUndelegateCoins(t *testing.T) {
origCoins := sdk.Coins{sdk.NewInt64Coin("steak", 100)}
delCoins := sdk.Coins{sdk.NewInt64Coin("steak", 50)}
bankKeeper := NewBaseKeeper(input.ak, input.pk.Subspace(DefaultParamspace), DefaultCodespace)
bankKeeper.SetSendEnabled(ctx, true)

addr1 := sdk.AccAddress([]byte("addr1"))
addr2 := sdk.AccAddress([]byte("addr2"))
Expand Down
9 changes: 1 addition & 8 deletions x/bank/params.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,9 @@ import (
const (
// DefaultParamspace for params keeper
DefaultParamspace = "bank"
// DefaultSendEnabled enabled
DefaultSendEnabled = true
)

// ParamStoreKeySendEnabled is store's key for SendEnabled
var ParamStoreKeySendEnabled = []byte("sendenabled")

// ParamKeyTable type declaration for parameters
func ParamKeyTable() params.KeyTable {
return params.NewKeyTable(
ParamStoreKeySendEnabled, false,
)
return params.NewKeyTable()
}
4 changes: 0 additions & 4 deletions x/gov/endblocker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ func TestTickExpiredDepositPeriod(t *testing.T) {
mapp, keeper, _, addrs, _, _ := getMockApp(t, 10, GenesisState{}, nil)
mapp.BeginBlock(abci.RequestBeginBlock{})
ctx := mapp.BaseApp.NewContext(false, abci.Header{})
keeper.ck.SetSendEnabled(ctx, true)
govHandler := NewHandler(keeper)

inactiveQueue := keeper.InactiveProposalQueueIterator(ctx, ctx.BlockHeader().Time)
Expand Down Expand Up @@ -58,7 +57,6 @@ func TestTickMultipleExpiredDepositPeriod(t *testing.T) {
mapp, keeper, _, addrs, _, _ := getMockApp(t, 10, GenesisState{}, nil)
mapp.BeginBlock(abci.RequestBeginBlock{})
ctx := mapp.BaseApp.NewContext(false, abci.Header{})
keeper.ck.SetSendEnabled(ctx, true)
govHandler := NewHandler(keeper)

inactiveQueue := keeper.InactiveProposalQueueIterator(ctx, ctx.BlockHeader().Time)
Expand Down Expand Up @@ -115,7 +113,6 @@ func TestTickPassedDepositPeriod(t *testing.T) {
mapp, keeper, _, addrs, _, _ := getMockApp(t, 10, GenesisState{}, nil)
mapp.BeginBlock(abci.RequestBeginBlock{})
ctx := mapp.BaseApp.NewContext(false, abci.Header{})
keeper.ck.SetSendEnabled(ctx, true)
govHandler := NewHandler(keeper)

inactiveQueue := keeper.InactiveProposalQueueIterator(ctx, ctx.BlockHeader().Time)
Expand Down Expand Up @@ -158,7 +155,6 @@ func TestTickPassedVotingPeriod(t *testing.T) {
SortAddresses(addrs)
mapp.BeginBlock(abci.RequestBeginBlock{})
ctx := mapp.BaseApp.NewContext(false, abci.Header{})
keeper.ck.SetSendEnabled(ctx, true)
govHandler := NewHandler(keeper)

inactiveQueue := keeper.InactiveProposalQueueIterator(ctx, ctx.BlockHeader().Time)
Expand Down
1 change: 0 additions & 1 deletion x/gov/expected_keepers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,4 @@ type BankKeeper interface {

// TODO remove once governance doesn't require use of accounts
SendCoins(ctx sdk.Context, fromAddr sdk.AccAddress, toAddr sdk.AccAddress, amt sdk.Coins) (sdk.Tags, sdk.Error)
SetSendEnabled(ctx sdk.Context, enabled bool)
}