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

Added a "new_shares" attribute inside the delegate event type #9214

Merged
merged 3 commits into from
Apr 29, 2021
Merged

Added a "new_shares" attribute inside the delegate event type #9214

merged 3 commits into from
Apr 29, 2021

Conversation

RiccardoM
Copy link
Contributor

Description

This PR adds a new attribute inside the event types delegate. This is useful to return the new shares amount that cannot be queried otherwise without having to perform a new gRPC query.

The rationale behind this is to allow state parsers (such as BDJuno) to handle properly the MsgDelegate parsing, storing a new row that represents the delegation and its associated shares amount.

Currently, in order to do so we have to query the gRPC to get the shares amount. With this change, this won't be needed anymore as the new shares amount will be returned inside the event itself.

closes: #XXXX


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 Apr 28, 2021

Codecov Report

Merging #9214 (e46d7d6) into master (6ad84c5) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9214      +/-   ##
==========================================
- Coverage   60.03%   60.03%   -0.01%     
==========================================
  Files         595      595              
  Lines       37352    37353       +1     
==========================================
  Hits        22425    22425              
- Misses      12939    12940       +1     
  Partials     1988     1988              
Impacted Files Coverage Δ
x/staking/keeper/msg_server.go 0.45% <0.00%> (-0.01%) ⬇️

Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

okay for me

@amaury1093
Copy link
Contributor

Could you add a changelog entry?

@RiccardoM
Copy link
Contributor Author

Could you add a changelog entry?

What tag should I use? And under which section?

@amaury1093
Copy link
Contributor

Unreleased > Features seems the most logical to me

@cyberbono3 cyberbono3 self-requested a review April 29, 2021 04:05
Copy link
Contributor

@cyberbono3 cyberbono3 left a comment

Choose a reason for hiding this comment

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

LGTM

@RiccardoM
Copy link
Contributor Author

Unreleased > Features seems the most logical to me

Added 👍

@tac0turtle tac0turtle merged commit afa818a into cosmos:master Apr 29, 2021
@alexanderbez
Copy link
Contributor

We explicitly removed the concept of "shares" from all UX surface long ago. Was there a reason you needed this @RiccardoM?

@RiccardoM RiccardoM deleted the delegate-new-shares-event-atribute branch April 29, 2021 12:53
@RiccardoM
Copy link
Contributor Author

We explicitly removed the concept of "shares" from all UX surface long ago. Was there a reason you needed this @RiccardoM?

Many explorers still use this to calculate the self delegate ratio of a validator. If you think there's a better way to do this, then this commit can get reverted

@alexanderbez
Copy link
Contributor

Why can you not take their self-delegation / total-delegation?

@RiccardoM
Copy link
Contributor Author

Why can you not take their self-delegation / total-delegation?

Actually, you're right. I thought that shares were still useful to determine whether a validator was slashes in the past or not (since the share/token ratio would increase in that case). But I think we can simply get the past events and see if there was a slashing event I guess.

Feel free to revert this change if you think it's useless and/or might make things more complicated UX-wise

@alexanderbez
Copy link
Contributor

Ehh there's no harm :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants