Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Add Deposit and Withdraw Events to Balances Pallet #9425

Merged
29 commits merged into from
Oct 11, 2021
Merged

Conversation

apopiak
Copy link
Contributor

@apopiak apopiak commented Jul 23, 2021

addresses paritytech/polkadot#3508

[The missing events] make it difficult to trace transfers when these functions are called e.g. via XCM. (As surfaced by paritytech/polkadot#3489)

This PR adds Deposit, Withdraw and Slashed events as well as deposit_event calls where appropriate in order to make it easier to trace balance movements.

+ add deposit_event() calls where appropriate to signal fund movement
+ adjust and extend tests
@apopiak apopiak added A0-please_review Pull request needs code review. B3-apinoteworthy C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Jul 23, 2021
@apopiak apopiak added this to In progress in Runtime via automation Jul 23, 2021
@shawntabrizi
Copy link
Contributor

Can you add some justification why we need these events?

Seems to me we have been fine so far without them, and there is non-zero overhead of such things.

@apopiak
Copy link
Contributor Author

apopiak commented Jul 25, 2021

Added a justification based on the linked Polkadot issue.
TLDR: Right now it's impossible to know funds were transfered via XCM (if you only follow events).

bin/node/bench/src/import.rs Outdated Show resolved Hide resolved
@apopiak apopiak changed the title add Deposit and Withdraw events to balances Add Deposit and Withdraw Events to Balances Pallet Jul 27, 2021
@xlc
Copy link
Contributor

xlc commented Jul 28, 2021

Even with this, from payment receiver point of view, is it possible to know where the fund is coming from?

@apopiak
Copy link
Contributor Author

apopiak commented Jul 28, 2021

Even with this, from payment receiver point of view, is it possible to know where the fund is coming from?

Only if you track the fund movements on the source and target chains. As the receiver you would only see a Deposit which does not have a source.
We can explore having more events on the XCM side as well, that would need to be in Polkadot (and maybe Cumulus), though.

@xlc
Copy link
Contributor

xlc commented Jul 28, 2021

This is useful for exchanges to detect incoming deposits. But doesn't solve the problem of tracking funds. I guess we need some event system rework at some stage.

Still, we will want this get released ASAP otherwise it is impossible for exchanges to handling deposits via XCM.

@apopiak apopiak requested a review from athei as a code owner July 28, 2021 15:19
@stale stale bot removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Oct 7, 2021
Runtime automation moved this from In progress to Needs Audit Oct 7, 2021
@apopiak
Copy link
Contributor Author

apopiak commented Oct 11, 2021

bot merge

@ghost
Copy link

ghost commented Oct 11, 2021

Trying merge.

@ghost ghost merged commit 12b6441 into master Oct 11, 2021
@ghost ghost deleted the apopiak/balances-events branch October 11, 2021 14:23
Runtime automation moved this from Needs Audit to Done Oct 11, 2021
@TheGoldenEye
Copy link

In which runtime it will be active?

@shawntabrizi
Copy link
Contributor

I think 0912 it should get in

This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants