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

Return an unsigned tx in legacy GET /tx endpoint when signature conversion fails #7649

Merged
merged 8 commits into from
Oct 23, 2020
Merged

Return an unsigned tx in legacy GET /tx endpoint when signature conversion fails #7649

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0defd23
Return an unsigned tx in legacy GET /tx endpoint when signature conve…
aaronc Oct 23, 2020
f703f62
Add test
aaronc Oct 23, 2020
22e27b8
add comment
aaronc Oct 23, 2020
89e1b6e
add comment
aaronc Oct 23, 2020
8ccc784
Merge branch 'master' into aaronc/7639-ignore-sig-error
aaronc Oct 23, 2020
a1a97d8
add comment
aaronc Oct 23, 2020
a00a4b1
Merge remote-tracking branch 'origin/aaronc/7639-ignore-sig-error' in…
aaronc Oct 23, 2020
1538cee
Merge branch 'master' into aaronc/7639-ignore-sig-error
aaronc Oct 23, 2020
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
18 changes: 13 additions & 5 deletions client/tx/legacy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ func ConvertTxToStdTx(codec *codec.LegacyAmino, tx signing.Tx) (legacytx.StdTx,
aminoTxConfig := legacytx.StdTxConfig{Cdc: codec}
builder := aminoTxConfig.NewTxBuilder()

err := CopyTx(tx, builder)
err := CopyTx(tx, builder, true)
if err != nil {

return legacytx.StdTx{}, err
Expand All @@ -34,8 +34,9 @@ func ConvertTxToStdTx(codec *codec.LegacyAmino, tx signing.Tx) (legacytx.StdTx,
}

// CopyTx copies a Tx to a new TxBuilder, allowing conversion between
// different transaction formats.
func CopyTx(tx signing.Tx, builder client.TxBuilder) error {
// different transaction formats. If ignoreSignatureError is true, copying will continue
// tx even if the signature cannot be set in the target builder resulting in an unsigned tx.
func CopyTx(tx signing.Tx, builder client.TxBuilder, ignoreSignatureError bool) error {
err := builder.SetMsgs(tx.GetMsgs()...)
if err != nil {
return err
Expand All @@ -48,7 +49,13 @@ func CopyTx(tx signing.Tx, builder client.TxBuilder) error {

err = builder.SetSignatures(sigs...)
if err != nil {
return err
if ignoreSignatureError {
// we call SetSignatures() agan with no args to clear any signatures in case the
// previous call to SetSignatures() had any partial side-effects
_ = builder.SetSignatures()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why we are calling this second time?

Copy link
Member Author

Choose a reason for hiding this comment

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

In case there is any side effect of the initial SetSignatures. For instance maybe there were 2 signatures and 1 was successful but the other wasn't. This call clears all signatures.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's already merged, but I still think we should add a comment.

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 did

} else {
return err
}
}

builder.SetMemo(tx.GetMemo())
Expand All @@ -58,6 +65,7 @@ func CopyTx(tx signing.Tx, builder client.TxBuilder) error {
return nil
}

// ConvertAndEncodeStdTx encodes the stdTx as a transaction in the format specified by txConfig
func ConvertAndEncodeStdTx(txConfig client.TxConfig, stdTx legacytx.StdTx) ([]byte, error) {
builder := txConfig.NewTxBuilder()

Expand All @@ -67,7 +75,7 @@ func ConvertAndEncodeStdTx(txConfig client.TxConfig, stdTx legacytx.StdTx) ([]by
if _, ok := builder.GetTx().(legacytx.StdTx); ok {
theTx = stdTx
} else {
err := CopyTx(stdTx, builder)
err := CopyTx(stdTx, builder, false)
if err != nil {
return nil, err
}
Expand Down
27 changes: 22 additions & 5 deletions client/tx/legacy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,10 +68,10 @@ func (s *TestSuite) TestCopyTx() {
protoBuilder := s.protoCfg.NewTxBuilder()
buildTestTx(s.T(), protoBuilder)
aminoBuilder := s.aminoCfg.NewTxBuilder()
err := tx2.CopyTx(protoBuilder.GetTx(), aminoBuilder)
err := tx2.CopyTx(protoBuilder.GetTx(), aminoBuilder, false)
s.Require().NoError(err)
protoBuilder2 := s.protoCfg.NewTxBuilder()
err = tx2.CopyTx(aminoBuilder.GetTx(), protoBuilder2)
err = tx2.CopyTx(aminoBuilder.GetTx(), protoBuilder2, false)
s.Require().NoError(err)
bz, err := s.protoCfg.TxEncoder()(protoBuilder.GetTx())
s.Require().NoError(err)
Expand All @@ -83,10 +83,10 @@ func (s *TestSuite) TestCopyTx() {
aminoBuilder = s.aminoCfg.NewTxBuilder()
buildTestTx(s.T(), aminoBuilder)
protoBuilder = s.protoCfg.NewTxBuilder()
err = tx2.CopyTx(aminoBuilder.GetTx(), protoBuilder)
err = tx2.CopyTx(aminoBuilder.GetTx(), protoBuilder, false)
s.Require().NoError(err)
aminoBuilder2 := s.aminoCfg.NewTxBuilder()
err = tx2.CopyTx(protoBuilder.GetTx(), aminoBuilder2)
err = tx2.CopyTx(protoBuilder.GetTx(), aminoBuilder2, false)
s.Require().NoError(err)
bz, err = s.aminoCfg.TxEncoder()(aminoBuilder.GetTx())
s.Require().NoError(err)
Expand All @@ -108,6 +108,23 @@ func (s *TestSuite) TestConvertTxToStdTx() {
s.Require().Equal(sig.PubKey, stdTx.Signatures[0].PubKey)
s.Require().Equal(sig.Data.(*signing2.SingleSignatureData).Signature, stdTx.Signatures[0].Signature)

// SIGN_MODE_DIRECT should fall back to an unsigned tx
err = protoBuilder.SetSignatures(signing2.SignatureV2{
PubKey: pub1,
Data: &signing2.SingleSignatureData{
SignMode: signing2.SignMode_SIGN_MODE_DIRECT,
Signature: []byte("dummy"),
},
})
s.Require().NoError(err)
stdTx, err = tx2.ConvertTxToStdTx(s.encCfg.Amino, protoBuilder.GetTx())
s.Require().NoError(err)
s.Require().Equal(memo, stdTx.Memo)
s.Require().Equal(gas, stdTx.Fee.Gas)
s.Require().Equal(fee, stdTx.Fee.Amount)
s.Require().Equal(msg, stdTx.Msgs[0])
s.Require().Empty(stdTx.Signatures)

// std tx
aminoBuilder := s.aminoCfg.NewTxBuilder()
buildTestTx(s.T(), aminoBuilder)
Expand All @@ -127,7 +144,7 @@ func (s *TestSuite) TestConvertAndEncodeStdTx() {
decodedTx, err := s.protoCfg.TxDecoder()(txBz)
s.Require().NoError(err)
aminoBuilder2 := s.aminoCfg.NewTxBuilder()
s.Require().NoError(tx2.CopyTx(decodedTx.(signing.Tx), aminoBuilder2))
s.Require().NoError(tx2.CopyTx(decodedTx.(signing.Tx), aminoBuilder2, false))
s.Require().Equal(stdTx, aminoBuilder2.GetTx())

// just use amino everywhere
Expand Down