-
Notifications
You must be signed in to change notification settings - Fork 295
Add CurrencyAdapter #262
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 CurrencyAdapter #262
Conversation
xlc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should have some more tests for imbalance creation (via withdraw / deposit) and non-trivial Currency method implementation.
shaunxw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nitpicks. And could you add a unit test for the pallet currency adapter, just a basic transfer should be fine.
tokens/src/lib.rs
Outdated
| fn slash(currency_id: Self::CurrencyId, who: &T::AccountId, amount: Self::Balance) -> Self::Balance { | ||
| if amount.is_zero() { | ||
| return amount; | ||
| return Zero::zero(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, why creating a new Zero value instead of just return amount as zero?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refer here. You are right, I'll recover it.
| } | ||
|
|
||
| type AccountId = u128; | ||
| type AccountId = u64; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why changing it to u64?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
impl Contains<u64> for TenToFourteen {
Other defines are u64, it is more convenient to modify here
| type KickedMember = (); | ||
| type BadReport = (); | ||
| type WeightInfo = (); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I missed something, but didn't see any usage for the new added mocks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because pallet_elections_phragmen and pallet_treasury module dispatch is private function. Maybe I can test it from genesis config.
Closes: #247