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

Stargate: Add sender info to bank transfer event #6553

Conversation

ethanfrey
Copy link
Contributor

Description

This is the "forward port" of #6552 (on the master branch). See description and discussion there please.


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

@codecov
Copy link

codecov bot commented Jun 30, 2020

Codecov Report

Merging #6553 into master will increase coverage by 1.27%.
The diff coverage is 67.03%.

@@            Coverage Diff             @@
##           master    #6553      +/-   ##
==========================================
+ Coverage   55.60%   56.87%   +1.27%     
==========================================
  Files         457      479      +22     
  Lines       27440    28797    +1357     
==========================================
+ Hits        15257    16379    +1122     
- Misses      11083    11261     +178     
- Partials     1100     1157      +57     

@alexanderbez alexanderbez added A:automerge Automatically merge PR once all prerequisites pass. R4R C:x/bank labels Jun 30, 2020
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

ACK

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

@ethanfrey can you please update the events.md doc to include the new event too please?

@ethanfrey
Copy link
Contributor Author

I just looked at events.md and it seems to document the behavior I add in this PR (not the previous behavior): https://github.com/cosmos/cosmos-sdk/blob/968fb1f040c5de58eb3dd80ded4e060c771031d1/docs/core/events.md#subscribing-to-events

For example, a transfer transaction triggers an event of type Transfer and has Recipient and Sender as attributes
...
"query": "tm.event='Tx' AND transfer.sender='senderAddress'"

Something that never worked until now. I couldn't find another events.md file and the link to events.go doesn't show me any docs to update. Can you link me to exactly what needs updating?

Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

LGTM

@tac0turtle
Copy link
Member

tac0turtle commented Jul 1, 2020

pulled this pr into #6558 in order to update to master. closing this pr

next time please enable editing on prs that are created

@tac0turtle tac0turtle closed this Jul 1, 2020
@ethanfrey
Copy link
Contributor Author

next time please enable editing on prs that are created

I would love to. I checked the github help and it suggest I check a box that does not exist on the page I see in front of me.

Thanks for rebasing and merging.

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. C:x/bank
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants