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.

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

Conversation

aaronc
Copy link
Member

@aaronc aaronc commented Oct 23, 2020

ref #7639

This will cause the legacy REST GET /tx endpoint to return an unsigned transaction when the sign mode cannot be represented in amino JSON so that this endpoint fails more gracefully.

It WILL NOT fix all errors with the legacy GET /tx endpoint as some tx's (ex. IBC) CANNOT be represented in amino at all.


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@aaronc aaronc marked this pull request as draft October 23, 2020 14:45
@codecov
Copy link

codecov bot commented Oct 23, 2020

Codecov Report

Merging #7649 into master will decrease coverage by 0.00%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #7649      +/-   ##
==========================================
- Coverage   54.14%   54.14%   -0.01%     
==========================================
  Files         611      611              
  Lines       38556    38559       +3     
==========================================
  Hits        20877    20877              
- Misses      15547    15550       +3     
  Partials     2132     2132              

@@ -48,7 +49,11 @@ func CopyTx(tx signing.Tx, builder client.TxBuilder) error {

err = builder.SetSignatures(sigs...)
if err != nil {
return err
if ignoreSignatureError {
_ = 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

@zmanian
Copy link
Member

zmanian commented Oct 23, 2020

I am trying to figure out what exactly would happen when CopyTx hits an IBC message where amino isn't defined.

That would trigger the deprecation error message? how does that bubble up.

@aaronc
Copy link
Member Author

aaronc commented Oct 23, 2020

I am trying to figure out what exactly would happen when CopyTx hits an IBC message where amino isn't defined.

That would trigger the deprecation error message? how does that bubble up.

It would trigger an error in marshaling. That needs to be addressed, but IMHO separately (#7639)

@aaronc aaronc added the A:automerge Automatically merge PR once all prerequisites pass. label Oct 23, 2020
@mergify mergify bot merged commit 93ec7bb into master Oct 23, 2020
@mergify mergify bot deleted the aaronc/7639-ignore-sig-error branch October 23, 2020 19:20
clevinson pushed a commit that referenced this pull request Oct 29, 2020
…rsion fails (#7649)

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

* Add test

* add comment

* add comment

* add comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. T: Client UX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants