Skip to content

Events in tokens pallet do not completely cover balance changes #726

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

Closed
sea212 opened this issue Apr 8, 2022 · 5 comments · Fixed by #729
Closed

Events in tokens pallet do not completely cover balance changes #726

sea212 opened this issue Apr 8, 2022 · 5 comments · Fixed by #729
Assignees

Comments

@sea212
Copy link
Contributor

sea212 commented Apr 8, 2022

Since not every change of token balance is indicated with events, it becomes immensely difficult to correctly track tokens balances in a block indexer.
I suggest to offer a complete set of events that does indicate any changes in token balances like it's done in the balances pallet.

@xlc
Copy link
Member

xlc commented Apr 10, 2022

Can you point out what's missing? PR are always welcome.

@sea212
Copy link
Contributor Author

sea212 commented Apr 12, 2022

I think the only event that is missing is Slashed

/// Some amount was removed from the account (e.g. for misbehavior).
Slashed { who: T::AccountId, amount: T::Balance },

@xlc I can create the PR. As I see it now the event belongs into orml-currencies.
I am just wondering why are some events like

/// Currency transfer success.
Transferred {
	currency_id: CurrencyIdOf<T>,
	from: T::AccountId,
	to: T::AccountId,
	amount: BalanceOf<T>,
},
/// Update balance success.
BalanceUpdated {
	currency_id: CurrencyIdOf<T>,
	who: T::AccountId,
	amount: AmountOf<T>,
},
/// Deposit success.
Deposited {
	currency_id: CurrencyIdOf<T>,
	who: T::AccountId,
	amount: BalanceOf<T>,
},
/// Withdraw success.
Withdrawn {
	currency_id: CurrencyIdOf<T>,
	who: T::AccountId,
	amount: BalanceOf<T>,
},

emitted in orml-currencies? I think it should just do the calls to the respective trait implementations specified by Config types MultiCurrency and NativeCurrency and the implementations should be responsible for emitting the events. Otherwise multiple events that state the same will be emitted.

@xlc
Copy link
Member

xlc commented Apr 13, 2022

The original idea is that events are emitted by the extrinsic pallet (currencies), not the underlying implementation (tokens) so that people can decide what event to emit.

But I does see it could create some inconsistency.

@shaunxw
Copy link
Member

shaunxw commented Apr 13, 2022

I agree we should make it that only implementations emit events, and to cover all possible balance changes. In this way ORML users don't have to worry about adding events, and are always free to add custom ones if needed while wrapping these impl.

The current balance related events could be updated as:

  1. Add Slashed event to orml-tokens.
  2. In orml-tokens, deposit events in traits/pallet impl, instead of in dispatchable calls.
  3. Remove events in orml-currencies, to get rid of events deposit duplication.

@syan095
Copy link
Contributor

syan095 commented Apr 26, 2022

orml tokens now emits events whenever balance change occurs.
events are emitted in implementation level. Duplicate deposits are removed.
orml currencie's events have all been removed

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

Successfully merging a pull request may close this issue.

4 participants