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

Emit balance change events from the protocol #3141

Merged
merged 104 commits into from
May 9, 2024

Conversation

sug0
Copy link
Contributor

@sug0 sug0 commented Apr 26, 2024

Describe your changes

Closes #84

Emits balance change events for the following protocol actions:

  • Fee payments to validators
  • PGF payments (including over IBC)
  • Governance refunds
  • IBC token minting/burning
  • Proof-of-stake validator slashing
  • WASM transfers

Indicate on which release or other PRs this topic is based on

#3102

Diff for review: https://github.com/anoma/namada/compare/tiago/move-events-to-submodules..tiago/balance-change-events (https://github.com/anoma/namada/pull/3141/files/68420a57f251ca8e68ae6960a4a0081e57c16403..406f715b416b069b310410fec20f0ba0cb7c4a06)

Checklist before merging to draft

  • I have added a changelog
  • Git history is in acceptable state

@sug0 sug0 added enhancement New feature or request breaking-change labels Apr 26, 2024
Copy link

codecov bot commented Apr 26, 2024

Codecov Report

Attention: Patch coverage is 63.73057% with 770 lines in your changes are missing coverage. Please review.

Project coverage is 59.65%. Comparing base (ea843f7) to head (50dc3c7).
Report is 21 commits behind head on main.

Files Patch % Lines
crates/ibc/src/event.rs 46.94% 113 Missing ⚠️
...tes/apps/src/lib/node/ledger/shell/testing/node.rs 0.00% 88 Missing ⚠️
crates/events/src/extend.rs 74.26% 88 Missing ⚠️
...rates/apps/src/lib/node/ledger/shell/governance.rs 14.13% 79 Missing ⚠️
crates/events/src/lib.rs 72.68% 62 Missing ⚠️
crates/governance/src/event.rs 52.21% 54 Missing ⚠️
crates/sdk/src/rpc.rs 0.00% 32 Missing ⚠️
crates/state/src/wl_state.rs 0.00% 30 Missing ⚠️
...s/apps/src/lib/node/ledger/shell/finalize_block.rs 29.41% 24 Missing ⚠️
crates/ibc/src/actions.rs 0.00% 20 Missing ⚠️
... and 25 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3141      +/-   ##
==========================================
+ Coverage   59.41%   59.65%   +0.24%     
==========================================
  Files         298      303       +5     
  Lines       92326    93349    +1023     
==========================================
+ Hits        54853    55689     +836     
- Misses      37473    37660     +187     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sug0 sug0 requested a review from grarco April 26, 2024 15:56
@sug0 sug0 marked this pull request as ready for review April 30, 2024 14:05
@brentstone
Copy link
Collaborator

FWIW, currently slashing does not actually involve any balance changes. The tokens are held in the PoS account and they are kept there, only other voting power-related data is manipulated. In PoS, only bonding and withdrawing involve balance changes.

I don't think I know enough about the event emission in general to say whether or not we should still emit events for slashing or if they should strictly be done for balance changes in the context of this PR though.

@sug0
Copy link
Contributor Author

sug0 commented May 2, 2024

@brentstone yeah, slashing events were loosely grouped together with other balance change events in this pr, but they're defined as Proof-of-Stake events, rather than Token events. I think it's still useful to emit them in the protocol, as long as we distinguish them from balance change events.

@Fraccaman
Copy link
Member

my only comment is that we could move each TokenTransfer event to a static method to avoid having constant string around the code, but we can also do this refactor later. Anything else looks good to me!

@Fraccaman Fraccaman self-requested a review May 7, 2024 09:00
Fraccaman
Fraccaman previously approved these changes May 7, 2024
crates/token/src/lib.rs Outdated Show resolved Hide resolved
tzemanovic
tzemanovic previously approved these changes May 7, 2024
@sug0 sug0 marked this pull request as draft May 7, 2024 10:06
@sug0 sug0 dismissed stale reviews from tzemanovic and Fraccaman via 50dc3c7 May 7, 2024 12:39
@sug0 sug0 force-pushed the tiago/balance-change-events branch from 406f715 to 50dc3c7 Compare May 7, 2024 12:39
@sug0 sug0 marked this pull request as ready for review May 7, 2024 12:39
brentstone added a commit that referenced this pull request May 8, 2024
* origin/tiago/balance-change-events:
  Changelog for #3141
  Fix Multitoken native VP err msg
  Log which token failed to be minted on IBC native VP
  Fix unit tests
  Emit token transfer event from wasm
  Emit PoS slashing events
  Add void event sink for testing
  Proof of stake events
  Move `token` events to `trans_token`
  Refactor token events
  Remove minted supply target
  Governance refund balance change events
  Burn IBC tokens and emit events
  Mint IBC tokens and emit events
  Remove unused file
  Implement EmitEvents on state impls
  Add token minting function
  Emit PGF payment events
  Add event level to balance change events
  Make the token event's post balance optional
  Extend event with a closure
  Refactor governace event emission
  Add more balance change targets
  Include wrapper tx hash in balance change event
  Include current height in wrapper fee payment event
  Emit fee payment balance change events
  Add token balance change events
  Negate I256 numbers
  Convert from Uint to Amount with 0 denom
  Implement FromStr on I256
@brentstone brentstone merged commit f61744a into main May 9, 2024
12 of 19 checks passed
@brentstone brentstone deleted the tiago/balance-change-events branch May 9, 2024 03:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

protocol balance change events
5 participants