Skip to content

Commit

Permalink
fix: remove hardcoded pubkey from tx simulation (cosmos#11282)
Browse files Browse the repository at this point in the history 
## Description

Closes: cosmos#11283



---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [x] provided a link to the relevant issue or specification
- [x] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [x] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [x] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed 
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
  • Loading branch information
@fedekunze
fedekunze committed Feb 28, 2022
1 parent e460f01 commit e657190
Show file tree
Hide file tree
Showing 3 changed files with 116 additions and 29 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Expand Up @@ -176,6 +176,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

### Bug Fixes

* (client) [\#11283](https://github.com/cosmos/cosmos-sdk/issues/11283) Support multiple keys for tx simulation and setting automatic gas for txs.
* (store) [\#11177](https://github.com/cosmos/cosmos-sdk/pull/11177) Update the prune `everything` strategy to store the last two heights.
* [\#10844](https://github.com/cosmos/cosmos-sdk/pull/10844) Automatic recovering non-consistent keyring storage during public key import.
* (store) [\#11117](https://github.com/cosmos/cosmos-sdk/pull/11117) Fix data race in store trace component
Expand Down
24 changes: 23 additions & 1 deletion client/tx/factory.go
View file
Expand Up @@ -11,6 +11,7 @@ import (
"github.com/cosmos/cosmos-sdk/client/flags"
"github.com/cosmos/cosmos-sdk/crypto/keyring"
"github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1"
cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/tx"
"github.com/cosmos/cosmos-sdk/types/tx/signing"
Expand Down Expand Up @@ -306,10 +307,31 @@ func (f Factory) BuildSimTx(msgs ...sdk.Msg) ([]byte, error) {
return nil, err
}

// use the first element from the list of keys in order to generate a valid
// pubkey that supports multiple algorithms

var (
ok bool
pk cryptotypes.PubKey = &secp256k1.PubKey{} // use default public key type
)

if f.keybase != nil {
records, _ := f.keybase.List()
if len(records) == 0 {
return nil, errors.New("cannot build signature for simulation, key records slice is empty")
}

// take the first record just for simulation purposes
pk, ok = records[0].PubKey.GetCachedValue().(cryptotypes.PubKey)
if !ok {
return nil, errors.New("cannot build signature for simulation, failed to convert proto Any to public key")
}
}

// Create an empty signature literal as the ante handler will populate with a
// sentinel pubkey.
sig := signing.SignatureV2{
PubKey: &secp256k1.PubKey{},
PubKey: pk,
Data: &signing.SingleSignatureData{
SignMode: f.signMode,
},
Expand Down
120 changes: 92 additions & 28 deletions client/tx/tx_test.go
View file
Expand Up @@ -45,6 +45,7 @@ func (m mockContext) Invoke(grpcCtx gocontext.Context, method string, req, reply

return nil
}

func (mockContext) NewStream(gocontext.Context, *grpc.StreamDesc, string, ...grpc.CallOption) (grpc.ClientStream, error) {
panic("not implemented")
}
Expand Down Expand Up @@ -96,6 +97,14 @@ func TestCalculateGas(t *testing.T) {

func TestBuildSimTx(t *testing.T) {
txCfg := NewTestTxConfig()
encCfg := simapp.MakeTestEncodingConfig()

kb, err := keyring.New(t.Name(), "test", t.TempDir(), nil, encCfg.Codec)
require.NoError(t, err)

path := hd.CreateHDPath(118, 0, 0).String()
_, _, err = kb.NewMnemonic("test_key1", keyring.English, path, keyring.DefaultBIP39Passphrase, hd.Secp256k1)
require.NoError(t, err)

txf := tx.Factory{}.
WithTxConfig(txCfg).
Expand All @@ -104,7 +113,8 @@ func TestBuildSimTx(t *testing.T) {
WithFees("50stake").
WithMemo("memo").
WithChainID("test-chain").
WithSignMode(txCfg.SignModeHandler().DefaultMode())
WithSignMode(txCfg.SignModeHandler().DefaultMode()).
WithKeybase(kb)

msg := banktypes.NewMsgSend(sdk.AccAddress("from"), sdk.AccAddress("to"), nil)
bz, err := txf.BuildSimTx(msg)
Expand All @@ -113,13 +123,23 @@ func TestBuildSimTx(t *testing.T) {
}

func TestBuildUnsignedTx(t *testing.T) {
encCfg := simapp.MakeTestEncodingConfig()
kb, err := keyring.New(t.Name(), "test", t.TempDir(), nil, encCfg.Codec)
require.NoError(t, err)

path := hd.CreateHDPath(118, 0, 0).String()

_, _, err = kb.NewMnemonic("test_key1", keyring.English, path, keyring.DefaultBIP39Passphrase, hd.Secp256k1)
require.NoError(t, err)

txf := tx.Factory{}.
WithTxConfig(NewTestTxConfig()).
WithAccountNumber(50).
WithSequence(23).
WithFees("50stake").
WithMemo("memo").
WithChainID("test-chain")
WithChainID("test-chain").
WithKeybase(kb)

msg := banktypes.NewMsgSend(sdk.AccAddress("from"), sdk.AccAddress("to"), nil)
tx, err := txf.BuildUnsignedTx(msg)
Expand All @@ -138,8 +158,8 @@ func TestSign(t *testing.T) {
kb, err := keyring.New(t.Name(), "test", t.TempDir(), nil, encCfg.Codec)
requireT.NoError(err)

var from1 = "test_key1"
var from2 = "test_key2"
from1 := "test_key1"
from2 := "test_key2"

// create a new key using a mnemonic generator and test if we can reuse seed to recreate that account
_, seed, err := kb.NewMnemonic(from1, keyring.English, path, keyring.DefaultBIP39Passphrase, hd.Secp256k1)
Expand Down Expand Up @@ -192,37 +212,81 @@ func TestSign(t *testing.T) {
expectedPKs []cryptotypes.PubKey
matchingSigs []int // if not nil, check matching signature against old ones.
}{
{"should fail if txf without keyring",
txfNoKeybase, txb, from1, true, nil, nil},
{"should fail for non existing key",
txfAmino, txb, "unknown", true, nil, nil},
{"amino: should succeed with keyring",
txfAmino, txbSimple, from1, true, []cryptotypes.PubKey{pubKey1}, nil},
{"direct: should succeed with keyring",
txfDirect, txbSimple, from1, true, []cryptotypes.PubKey{pubKey1}, nil},
{
"should fail if txf without keyring",
txfNoKeybase, txb, from1, true, nil, nil,
},
{
"should fail for non existing key",
txfAmino, txb, "unknown", true, nil, nil,
},
{
"amino: should succeed with keyring",
txfAmino, txbSimple, from1, true,
[]cryptotypes.PubKey{pubKey1},
nil,
},
{
"direct: should succeed with keyring",
txfDirect, txbSimple, from1, true,
[]cryptotypes.PubKey{pubKey1},
nil,
},

/**** test double sign Amino mode ****/
{"amino: should sign multi-signers tx",
txfAmino, txb, from1, true, []cryptotypes.PubKey{pubKey1}, nil},
{"amino: should append a second signature and not overwrite",
txfAmino, txb, from2, false, []cryptotypes.PubKey{pubKey1, pubKey2}, []int{0, 0}},
{"amino: should overwrite a signature",
txfAmino, txb, from2, true, []cryptotypes.PubKey{pubKey2}, []int{1, 0}},
{
"amino: should sign multi-signers tx",
txfAmino, txb, from1, true,
[]cryptotypes.PubKey{pubKey1},
nil,
},
{
"amino: should append a second signature and not overwrite",
txfAmino, txb, from2, false,
[]cryptotypes.PubKey{pubKey1, pubKey2},
[]int{0, 0},
},
{
"amino: should overwrite a signature",
txfAmino, txb, from2, true,
[]cryptotypes.PubKey{pubKey2},
[]int{1, 0},
},

/**** test double sign Direct mode
signing transaction with 2 or more DIRECT signers should fail in DIRECT mode ****/
{"direct: should append a DIRECT signature with existing AMINO",
{
"direct: should append a DIRECT signature with existing AMINO",
// txb already has 1 AMINO signature
txfDirect, txb, from1, false, []cryptotypes.PubKey{pubKey2, pubKey1}, nil},
{"direct: should add single DIRECT sig in multi-signers tx",
txfDirect, txb2, from1, false, []cryptotypes.PubKey{pubKey1}, nil},
{"direct: should fail to append 2nd DIRECT sig in multi-signers tx",
txfDirect, txb2, from2, false, []cryptotypes.PubKey{}, nil},
{"amino: should append 2nd AMINO sig in multi-signers tx with 1 DIRECT sig",
txfDirect, txb, from1, false,
[]cryptotypes.PubKey{pubKey2, pubKey1},
nil,
},
{
"direct: should add single DIRECT sig in multi-signers tx",
txfDirect, txb2, from1, false,
[]cryptotypes.PubKey{pubKey1},
nil,
},
{
"direct: should fail to append 2nd DIRECT sig in multi-signers tx",
txfDirect, txb2, from2, false,
[]cryptotypes.PubKey{},
nil,
},
{
"amino: should append 2nd AMINO sig in multi-signers tx with 1 DIRECT sig",
// txb2 already has 1 DIRECT signature
txfAmino, txb2, from2, false, []cryptotypes.PubKey{}, nil},
{"direct: should overwrite multi-signers tx with DIRECT sig",
txfDirect, txb2, from1, true, []cryptotypes.PubKey{pubKey1}, nil},
txfAmino, txb2, from2, false,
[]cryptotypes.PubKey{},
nil,
},
{
"direct: should overwrite multi-signers tx with DIRECT sig",
txfDirect, txb2, from1, true,
[]cryptotypes.PubKey{pubKey1},
nil,
},
}

var prevSigs []signingtypes.SignatureV2
Expand Down

0 comments on commit e657190

Please sign in to comment.