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

{Add,Reclaim}EscrowEvent: include share amounts #3944

Merged
merged 1 commit into from
May 14, 2021

Conversation

ptrus
Copy link
Member

@ptrus ptrus commented May 14, 2021

Fixes: #3939

Also fixed Escrow events emitted when disbursing rewards. I think explicitly emitting (transfer and self-escrow) events makes more sense?

@ptrus ptrus added the c:breaking Category: breaking code change label May 14, 2021
@ptrus ptrus force-pushed the ptrus/feature/staking-escrow-events branch from 71f385f to 41834da Compare May 14, 2021 09:56
docs/consensus/staking.md Outdated Show resolved Hide resolved
docs/consensus/staking.md Outdated Show resolved Hide resolved
@@ -0,0 +1,13 @@
Update emitted events when disbursing rewards
Copy link
Member

Choose a reason for hiding this comment

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

This may break integrations for custody providers which are parsing events for rewards. Should reclassify as breaking.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed. But do we want this change right? An alternative is also keeping it as it is and just having it documented

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think it makes tracking rewards more clear. We should just give a heads up when we release this.

@ptrus ptrus force-pushed the ptrus/feature/staking-escrow-events branch from 41834da to 7c50e7c Compare May 14, 2021 10:15
@codecov
Copy link

codecov bot commented May 14, 2021

Codecov Report

Merging #3944 (7c50e7c) into master (9278e19) will increase coverage by 0.08%.
The diff coverage is 85.71%.

❗ Current head 7c50e7c differs from pull request most recent head 2ee7d39. Consider uploading reports for the commit 2ee7d39 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3944      +/-   ##
==========================================
+ Coverage   67.04%   67.13%   +0.08%     
==========================================
  Files         406      406              
  Lines       41300    41351      +51     
==========================================
+ Hits        27689    27759      +70     
+ Misses       9667     9651      -16     
+ Partials     3944     3941       -3     
Impacted Files Coverage Δ
go/staking/api/api.go 58.87% <40.00%> (ø)
...o/consensus/tendermint/apps/staking/state/state.go 63.44% <85.93%> (+2.14%) ⬆️
.../consensus/tendermint/apps/staking/transactions.go 67.85% <87.50%> (+0.26%) ⬆️
go/consensus/tendermint/apps/staking/staking.go 53.16% <100.00%> (+0.60%) ⬆️
go/staking/tests/tester.go 91.82% <100.00%> (+0.11%) ⬆️
go/consensus/tendermint/apps/beacon/beacon.go 71.73% <0.00%> (-4.35%) ⬇️
go/consensus/tendermint/full/services.go 75.83% <0.00%> (-4.17%) ⬇️
go/consensus/tendermint/full/light.go 48.00% <0.00%> (-4.00%) ⬇️
go/worker/common/committee/node.go 71.21% <0.00%> (-3.54%) ⬇️
go/consensus/tendermint/apps/beacon/state/state.go 71.42% <0.00%> (-3.18%) ⬇️
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 42272a3...2ee7d39. Read the comment docs.

@@ -617,6 +617,7 @@ type AddEscrowEvent struct {
Owner Address `json:"owner"`
Escrow Address `json:"escrow"`
Amount quantity.Quantity `json:"amount"`
NewShares quantity.Quantity `json:"new_shares"`
Copy link
Member

Choose a reason for hiding this comment

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

Fix field alignment.

@ptrus ptrus force-pushed the ptrus/feature/staking-escrow-events branch from 7c50e7c to 2ee7d39 Compare May 14, 2021 11:59
@ptrus ptrus enabled auto-merge May 14, 2021 12:49
@ptrus ptrus merged commit ade6136 into master May 14, 2021
@ptrus ptrus deleted the ptrus/feature/staking-escrow-events branch May 14, 2021 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c:breaking Category: breaking code change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Staking event updates/improvements
2 participants