Skip to content

Conversation

@zjb0807
Copy link
Contributor

@zjb0807 zjb0807 commented Aug 26, 2020

Closes: #247

@zjb0807 zjb0807 requested a review from xlc August 26, 2020 00:58
@zjb0807 zjb0807 requested a review from xlc August 31, 2020 04:00
@xlc xlc requested a review from shaunxw August 31, 2020 04:01
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.

Should have some more tests for imbalance creation (via withdraw / deposit) and non-trivial Currency method implementation.

Copy link
Member

@shaunxw shaunxw left a 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.

fn slash(currency_id: Self::CurrencyId, who: &T::AccountId, amount: Self::Balance) -> Self::Balance {
if amount.is_zero() {
return amount;
return Zero::zero();
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

}

type AccountId = u128;
type AccountId = u64;
Copy link
Member

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?

Copy link
Contributor Author

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 = ();
}
Copy link
Member

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.

Copy link
Contributor Author

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.

@zjb0807 zjb0807 requested a review from shaunxw September 2, 2020 00:29
@zjb0807 zjb0807 merged commit 12c5cdc into master Sep 2, 2020
@zjb0807 zjb0807 deleted the CurrencyAdapter branch September 2, 2020 01:37
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.

CurrencyAdapter

4 participants