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

Fungibles trait and impl for Assets pallet#8425

Merged
gavofyork merged 14 commits intomasterfrom
gav-assets-xcm
Mar 23, 2021
Merged

Fungibles trait and impl for Assets pallet#8425
gavofyork merged 14 commits intomasterfrom
gav-assets-xcm

Conversation

@gavofyork
Copy link
Member

@gavofyork gavofyork commented Mar 22, 2021

This is needed for the XCM integrations for the Assets pallet.

This also includes some minor alterations to CI mainly for the renaming of a few of the labels.

@gavofyork gavofyork added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. labels Mar 22, 2021
@gavofyork gavofyork requested a review from kianenigma as a code owner March 22, 2021 21:28
@gavofyork gavofyork requested a review from a team as a code owner March 23, 2021 09:50
@gavofyork
Copy link
Member Author

cc @s3krit


let ln2 = {
let ln2 = P::from_rational(LN2.deconstruct().into(), Perquintill::ACCURACY.into());
/// `ln(2)` expressed in as perquintillionth.
Copy link
Member Author

Choose a reason for hiding this comment

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

Just some minor refactoring to more tightly scope the const.

if dest == source || amount.is_zero() {
return Ok(())
}
if dest != source && !amount.is_zero() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Need to condition this across most of the function since I moved the deposit_event into here.

id: T::AssetId,
source: &T::AccountId,
dest: &T::AccountId,
source: T::AccountId,
Copy link
Member Author

Choose a reason for hiding this comment

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

Needed to avoid a clone on deposit_event now it's in here.

let origin = ensure_signed(origin)?;
let beneficiary = T::Lookup::lookup(beneficiary)?;

Asset::<T>::try_mutate(id, |maybe_details| {
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved into a utility function so it can be shared with the Fungibles impl.

Copy link
Contributor

@s3krit s3krit left a comment

Choose a reason for hiding this comment

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

CI/label changes look good

falloff: P,
) -> P {
if stake < ideal_stake {
if stake <= ideal_stake {
Copy link
Member Author

Choose a reason for hiding this comment

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

@thiolliere this really should be a trivial optimisation, but tests fail with the change.

Copy link
Contributor

Choose a reason for hiding this comment

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

because test are testing for ideal_stake from 0 to 1, when ideal_stake is 0 we should modify the division below into a checked_div, or do a special case in some way

Copy link
Member

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

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

looks good to me

@gavofyork gavofyork merged commit 307a3b6 into master Mar 23, 2021
@gavofyork gavofyork deleted the gav-assets-xcm branch March 23, 2021 13:10
hirschenberger pushed a commit to hirschenberger/substrate that referenced this pull request Apr 14, 2021
* Fungibles trait and impl for Assets pallet

* Comment & whitespace

* Fixes

* Fix up CI/CD for the new labels.

* New labels.

* Fix labels

* Fix labels

* Whitespace

* Bump impl version.

* Fix accidental change

* Fixes

* Questionable fix.

* Better benchmark
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. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants