Skip to content

feat(tokens): OnDeposit, OnTransfer, OnSlash hooks #815

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

Merged
merged 2 commits into from
Oct 17, 2022

Conversation

daniel-savu
Copy link
Contributor

@daniel-savu daniel-savu commented Oct 14, 2022

Adds configurable hooks to the Tokens pallet, for functions which emit Event::Deposited, Event::Transfer, Event::Slashed. A hook call is not added to slash_reserved_named(...), because that function calls slash_reserved(...) so the hook is run there.

These hooks enable tokenizing "positions" which require lazy operations to be executed each time they are modified. An example would be tokenizing one's nomination / stake. For Alice to send her nominated position (the right to claim the nominated tokens) to Bob, her nomination rewards have to first be collected. In this example, the transfer hook would collect the sender's rewards before the actual transfer occurs.

Copy link
Member

@xlc xlc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need more tests

@@ -890,6 +901,7 @@ impl<T: Config> Pallet<T> {
amount: T::Balance,
existence_requirement: ExistenceRequirement,
) -> DispatchResult {
T::OnTransfer::on_transfer(currency_id, from, to, amount)?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move this after the early return. on the case no event is emitted, no hook should be called.

}

/// Hook to run before transferring from an account to another.
pub trait OnTransfer<AccountId, CurrencyId, Balance> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you intent to allow OnTransfer to be able to cancel the transfer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally I wanted all of these hooks to be able to fail, in consequence cancelling the transfer / deposit / slash. The trait bound on slash() prevents such an implementation unfortunately, but the other two (on_deposit and on_transfer) are allowed to return a Result so that's what they do.

@codecov-commenter
Copy link

codecov-commenter commented Oct 16, 2022

Codecov Report

Merging #815 (5800c29) into master (f90248d) will increase coverage by 0.19%.
The diff coverage is 98.85%.

@@            Coverage Diff             @@
##           master     #815      +/-   ##
==========================================
+ Coverage   78.05%   78.24%   +0.19%     
==========================================
  Files         102      102              
  Lines       10041    10129      +88     
==========================================
+ Hits         7837     7925      +88     
  Misses       2204     2204              
Impacted Files Coverage Δ
asset-registry/src/mock/para.rs 69.23% <ø> (ø)
currencies/src/mock.rs 94.73% <ø> (ø)
payments/src/mock.rs 95.83% <ø> (ø)
xtokens/src/mock/para.rs 55.00% <ø> (ø)
xtokens/src/mock/para_relative_view.rs 24.24% <ø> (ø)
xtokens/src/mock/para_teleport.rs 11.76% <ø> (ø)
traits/src/currency.rs 39.58% <80.00%> (+4.69%) ⬆️
tokens/src/lib.rs 84.14% <100.00%> (+0.20%) ⬆️
tokens/src/mock.rs 57.14% <100.00%> (+8.99%) ⬆️
tokens/src/tests.rs 100.00% <100.00%> (ø)
... and 3 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@zjb0807 zjb0807 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@xlc xlc merged commit 2c48b62 into open-web3-stack:master Oct 17, 2022
@gregdhill gregdhill deleted the dan/mutation-hooks branch October 18, 2022 08:07
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 this pull request may close these issues.

4 participants