-
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: Tags Cleanup & Docs #3137
R4R: Tags Cleanup & Docs #3137
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #3137 +/- ##
===========================================
+ Coverage 55.03% 55.07% +0.04%
===========================================
Files 133 133
Lines 9438 9443 +5
===========================================
+ Hits 5194 5201 +7
+ Misses 3924 3922 -2
Partials 320 320 |
@cwgoes I decided to address #2448 into two PRs in the spirit of trying to keep our PRs small and tidy. Namely, this PR simply does some cleanup and more importantly creates tags/events documentation for each module. A second PR, #3171, will be to actually implement a simple event manager. So this is R4R. |
|
||
| Key | Value | | ||
|-----------|---------------------------| | ||
| delegator | {delegatorAccountAddress} | |
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.
Is there a reason why we don't tag every message with it's type? Or do we and that isn't mentioned here?
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.
We do:
# baseapp.go#L:624
tags = append(tags, sdk.MakeTag(sdk.TagAction, []byte(msg.Type())))
I will be changing this in #3171 to be message-type
instead of action
so I'd rather do that in the aforementioned PR (i.e have this merged first).
|
||
The governance module emits the following events/tags: | ||
|
||
## EndBlocker |
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.
What does this mean? All governance events are tagged with these?
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.
Exactly.
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.
A couple of small questions, but looks good to me!
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
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: