Skip to content
This repository was archived by the owner on Nov 30, 2021. It is now read-only.

Add API for compatible query of two types of transactions#683

Merged
fedekunze merged 10 commits intocosmos:developmentfrom
summerpro:query-transaction
Jan 11, 2021
Merged

Add API for compatible query of two types of transactions#683
fedekunze merged 10 commits intocosmos:developmentfrom
summerpro:query-transaction

Conversation

@summerpro
Copy link
Contributor

@summerpro summerpro commented Dec 31, 2020

Closes: #630

Description

For contributor use:

  • 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

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

fedekunze
fedekunze suggested changes Dec 31, 2020
View reviewed changes
Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

Looks good! See the first comment for another proposed approach

@summerpro summerpro requested a review from fedekunze December 31, 2020 16:34
@summerpro
Copy link
Contributor Author

summerpro commented Dec 31, 2020

why deploy contract test failed ?

@summerpro
Copy link
Contributor Author

summerpro commented Jan 8, 2021

@fedekunze Can this pr be merged?

fedekunze
fedekunze approved these changes Jan 11, 2021
View reviewed changes
Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

untested ACK

@fedekunze fedekunze merged commit 9ecd264 into cosmos:development Jan 11, 2021
@araskachoi
Copy link
Contributor

araskachoi commented Jan 12, 2021

What was this supposed to fix? I'm thinking that this was a way to interpret the EthereumTxMsg that was generated from a transaction using RPC when querying the tx hash using curl localhost:8545/txs/{HASH}...?
If so, I have just tried doing this and it still gives the parsing err (internal error: tx {HASH} not found). This is probably related to #630 and may require a better way of decoding the amino msg or removing the EtheremintMsg altogether.
Can we discuss? Thanks!

@zhongqiuwood zhongqiuwood mentioned this pull request Jan 13, 2021
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@noot noot Awaiting requested review from noot

1 more reviewer

@fedekunze fedekunze fedekunze approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Failed to query "transaction sent by MetaMask" through transaction hash

3 participants

@summerpro @araskachoi @fedekunze