namada icon indicating copy to clipboard operation
namada copied to clipboard

Emit balance change events from the protocol

Open sug0 opened this issue 9 months ago • 4 comments

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

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

sug0 avatar Apr 26 '24 15:04 sug0

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 :warning:
...tes/apps/src/lib/node/ledger/shell/testing/node.rs 0.00% 88 Missing :warning:
crates/events/src/extend.rs 74.26% 88 Missing :warning:
...rates/apps/src/lib/node/ledger/shell/governance.rs 14.13% 79 Missing :warning:
crates/events/src/lib.rs 72.68% 62 Missing :warning:
crates/governance/src/event.rs 52.21% 54 Missing :warning:
crates/sdk/src/rpc.rs 0.00% 32 Missing :warning:
crates/state/src/wl_state.rs 0.00% 30 Missing :warning:
...s/apps/src/lib/node/ledger/shell/finalize_block.rs 29.41% 24 Missing :warning:
crates/ibc/src/actions.rs 0.00% 20 Missing :warning:
... 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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Apr 26 '24 15:04 codecov[bot]

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.

brentstone avatar Apr 30 '24 23:04 brentstone

@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.

sug0 avatar May 02 '24 09:05 sug0

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 avatar May 07 '24 09:05 Fraccaman