-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
R4R: fix tx search #2842
R4R: fix tx search #2842
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #2842 +/- ##
==========================================
Coverage ? 56.34%
==========================================
Files ? 120
Lines ? 8418
Branches ? 0
==========================================
Hits ? 4743
Misses ? 3353
Partials ? 322 |
…edekunze/2819-fix-tx-search Merge develop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking through this makes sense, but I'd like to test this first. Will do so in a bit ⚡️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! And docs updated. It would be really nice to have a document that describes what tags are applied by each module and how to properly use this functionality, but maybe a followup PR?
Pulled this down, ran tests, as well as manually testing the functionality so the code gets a 👍 from me, but can we also fix this functionality in the CLI? There is a gaiacli query txs
that needs to get looked at too.
@jackzampolin I think you're referring to #1780 ?
Was that failing as well ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice @fedekunze! I left some minor feedback ⚡️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @alexanderbez & two minor comments - otherwise LGTM.
this is R4R again @alexanderbez @jackzampolin @cwgoes @gamarin2 |
…mos/cosmos-sdk into fedekunze/2819-fix-tx-search Merge branch
…edekunze/2819-fix-tx-search Merge develop
Addressed comments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love the docs update! This is working for me when I tested and all the changes have been incorporated!
…mos/cosmos-sdk into fedekunze/2819-fix-tx-search Merge branch
addressed comments and updated docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested ACK
Closes #2819
/txs
and on CLIgaiacli query txs
value
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.
Wrote tests
Updated relevant documentation (
docs/
)Added entries in
PENDING.md
with issue #rereviewed
Files changed
in the github PR explorerFor Admin Use: