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 Missing Events for Balances Pallet #6974

Open
muharem opened this issue Dec 20, 2024 · 9 comments · May be fixed by #7250
Open

Add Missing Events for Balances Pallet #6974

muharem opened this issue Dec 20, 2024 · 9 comments · May be fixed by #7250

Comments

@muharem
Copy link
Contributor

muharem commented Dec 20, 2024

There are cases in the Balances pallet where events are missing, which might impact systems relying on balance updates for accounting. The known cases include:

  1. fungible trait impl for Hold/Release operations:
    No events are emitted when a balance is placed on hold or released.
  2. Burn/Mint Events for Credit/Debt drops
    When Credit or Debt fungible types are dropped, the total issuance is adjusted, but no Burn or Mint events are published.

Please verify these cases and check if any other major events are missing in the Balances pallet.

@Doordashcon
Copy link
Contributor

Doordashcon commented Jan 20, 2025

Hello @muharem

As per my investigation:

Hold & Release Events

impl<T: Config<I>, I: 'static> fungible::MutateHold<T::AccountId> for Pallet<T, I> {
	fn done_hold(reason: &Self::Reason, who: &T::AccountId, amount: Self::Balance) {
        Self::deposit_event(Event::<T, I>::Holding { reason: *reason, who: who.clone(), amount });
    }
    fn done_release(reason: &Self::Reason, who: &T::AccountId, amount: Self::Balance) {
        Self::deposit_event(Event::<T, I>::Released { reason: *reason, who: who.clone(), amount });
    }
}

@Doordashcon Doordashcon linked a pull request Jan 20, 2025 that will close this issue
@muharem
Copy link
Contributor Author

muharem commented Jan 20, 2025

Hello @muharem

As per my investigation:

Hold & Release Events

impl<T: Config, I: 'static> fungible::MutateHold<T::AccountId> for Pallet<T, I> {
fn done_hold(reason: &Self::Reason, who: &T::AccountId, amount: Self::Balance) {
Self::deposit_event(Event::<T, I>::Holding { reason: *reason, who: who.clone(), amount });
}
fn done_release(reason: &Self::Reason, who: &T::AccountId, amount: Self::Balance) {
Self::deposit_event(Event::<T, I>::Released { reason: *reason, who: who.clone(), amount });
}
}

left comment in the PR. how about burn and mint?

@Doordashcon
Copy link
Contributor

Doordashcon commented Jan 25, 2025

left comment in the PR. how about burn and mint?

Burn

fn done_settle(who: &T::AccountId, amount: Self::Balance, dust: &Self::Balance) {
 	if dust.is_zero() {
 		Self::deposit_event(Event::<T, I>::Burned { who: who.clone(), amount: amount.clone() });
 	} else {
 		Self::deposit_event(Event::<T, I>::BurnedWithDust { who: who.clone(), amount: amount.clone(), dust: *dust});
 	}
 }

Mint

fn done_resolve(who: &T::AccountId, amount: Self::Balance) {
 	Self::deposit_event(Event::<T, I>::Minted { who: who.clone(), amount })
 }

@muharem looking for other instances of missing events.

@muharem
Copy link
Contributor Author

muharem commented Jan 28, 2025

@Doordashcon its not correct. settle is not burn and resolve is not mint. you can do both without burning and minting. also I think it does not really matter if you burn 10 as amount and 0.1 as dust. you just burned 10.1

@muharem
Copy link
Contributor Author

muharem commented Jan 29, 2025

@Doordashcon let me know if something is not clear. I think these events publishing should be integrated into the on drops impls of HandleImbalanceDrop trait. But please double check this.

@Doordashcon
Copy link
Contributor

@muharem

Burn/Mint Events for Credit/Debt drops
When Credit or Debt fungible types are dropped, the total issuance is adjusted, but no Burn or Mint events are published.

This a bit unclear, if the Drops handle TotalIssuance adjustments, should the event emitted be Burn and Mint or TotalIssuanceForced.

@muharem
Copy link
Contributor Author

muharem commented Feb 5, 2025

@muharem

Burn/Mint Events for Credit/Debt drops
When Credit or Debt fungible types are dropped, the total issuance is adjusted, but no Burn or Mint events are published.

This a bit unclear, if the Drops handle TotalIssuance adjustments, should the event emitted be Burn and Mint or TotalIssuanceForced.

in my option Burn and Mint. it is not forced. it seem like forced event ie meant specifically for that call under the root origin

@Yung-Beef
Copy link

Any update on this?

@Doordashcon
Copy link
Contributor

This week @Yung-Beef

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

Successfully merging a pull request may close this issue.

3 participants