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

[Enhancement] Use XCM V3 for initiate_teleport weight calc #2102

Merged
merged 9 commits into from
Feb 9, 2023
Merged

[Enhancement] Use XCM V3 for initiate_teleport weight calc #2102

merged 9 commits into from
Feb 9, 2023

Conversation

ruseinov
Copy link
Contributor

@ruseinov ruseinov commented Jan 17, 2023 •
edited
Loading

Fixes #1669

@ruseinov ruseinov self-assigned this Jan 17, 2023
@ruseinov ruseinov added A0-please_review Pull request needs code review. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited B1-note_worthy Changes should be noted in the release notes T6-XCM This PR/Issue is related to XCM. C1-low PR touches the given topic and has a low impact on builders. labels Jan 17, 2023
@ruseinov ruseinov added D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. T1-runtime This PR/Issue is related to the topic “runtime”. and removed D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Jan 31, 2023
KiChjang
KiChjang reviewed Feb 7, 2023
View reviewed changes
parachains/runtimes/assets/statemine/src/weights/xcm/mod.rs Outdated
Self::Wild(_) => weight.saturating_mul(MAX_ASSETS as u64),
Self::Wild(asset) => match asset {
All => weight.saturating_mul(MAX_ASSETS as u64),
AllOf { .. } => weight,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm actually still uncertain about this -- comments on AllOfCounted says that each individual instance of an NFT is considered a different class of asset, and so wouldn't it make more sense to use the number of NFT instances as an upper bound here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, good question. Let me check it out and come back to you.

Copy link
Contributor Author

@ruseinov ruseinov Feb 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, I think this makes sense. I'm not sure though where the NFT instance count should be coming from in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

So this requires you to actually look inside the holding register and determine just how many AssetInstances there are... We could match against fun in AllOf and see if it's a NonFungible, and if so, it would basically be weight * MaxAssetsIntoHolding::get(), otherwise it's just weight.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@KiChjang One thing that disturbs me a bit is the comment below, should we then do something like weight * MaxAssetsIntoHolding::get() * 2 to be safe?

	/// The maximum number of assets we target to have in the Holding Register at any one time.
	///
	/// NOTE: In the worse case, the Holding Register may contain up to twice as many assets as this
	/// and any benchmarks should take that into account.
	type MaxAssetsIntoHolding: Get<u32>;

@ruseinov
Copy link
Contributor Author

ruseinov commented Feb 9, 2023

bot rebase

@paritytech-processbot
Copy link

Rebased

@ruseinov
Copy link
Contributor Author

ruseinov commented Feb 9, 2023

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot paritytech-processbot bot merged commit 8230ec4 into master Feb 9, 2023
@paritytech-processbot paritytech-processbot bot deleted the ru/enhancement/iinitinit-teleport branch February 9, 2023 15:01
bkontur pushed a commit that referenced this pull request Feb 25, 2023
@ruseinov @bkontur
[Enhancement] Use XCM V3 for initiate_teleport weight calc (#2102)
4b59c92 
* [Enhancement] Use XCM V3 for initiate_teleport weight calc

* deref

* replicate in all the runtimes

* fmt

* better handling for AllOf

* fmt

* small type fix

* replicate the fix for all runtimes

---------

Co-authored-by: parity-processbot <>
paritytech-processbot bot pushed a commit that referenced this pull request Feb 27, 2023
@bkontur @ruseinov
Tests + backport fix for teleport_assets (ReceiveTeleportedAsset) (#2235
1889c39 
)

* Wwstmint test for ReceiveTeleportedAsset

* Missing fix for `weigh_multi_assets`

* Added tests for statemine/statemint

* [Enhancement] Use XCM V3 for initiate_teleport weight calc (#2102)

* [Enhancement] Use XCM V3 for initiate_teleport weight calc

* deref

* replicate in all the runtimes

* fmt

* better handling for AllOf

* fmt

* small type fix

* replicate the fix for all runtimes

---------

Co-authored-by: parity-processbot <>

* removed `frame_support::sp_tracing::try_init_simple();`

* Review fixes

* Removed `as u64`

---------

Co-authored-by: Roman Useinov <[email protected]>
EgorPopelyaev pushed a commit that referenced this pull request Feb 27, 2023
@bkontur @ruseinov @EgorPopelyaev
Tests + backport fix for teleport_assets (ReceiveTeleportedAsset) (#2235
0bef37f 
)

* Wwstmint test for ReceiveTeleportedAsset

* Missing fix for `weigh_multi_assets`

* Added tests for statemine/statemint

* [Enhancement] Use XCM V3 for initiate_teleport weight calc (#2102)

* [Enhancement] Use XCM V3 for initiate_teleport weight calc

* deref

* replicate in all the runtimes

* fmt

* better handling for AllOf

* fmt

* small type fix

* replicate the fix for all runtimes

---------

Co-authored-by: parity-processbot <>

* removed `frame_support::sp_tracing::try_init_simple();`

* Review fixes

* Removed `as u64`

---------

Co-authored-by: Roman Useinov <[email protected]>
@EgorPopelyaev EgorPopelyaev mentioned this pull request Feb 27, 2023
Merged
EgorPopelyaev pushed a commit that referenced this pull request Feb 27, 2023
@bkontur @ruseinov @EgorPopelyaev
Tests + backport fix for teleport_assets (ReceiveTeleportedAsset) (#2235
e6a15c3 
)

* Wwstmint test for ReceiveTeleportedAsset

* Missing fix for `weigh_multi_assets`

* Added tests for statemine/statemint

* [Enhancement] Use XCM V3 for initiate_teleport weight calc (#2102)

* [Enhancement] Use XCM V3 for initiate_teleport weight calc

* deref

* replicate in all the runtimes

* fmt

* better handling for AllOf

* fmt

* small type fix

* replicate the fix for all runtimes

---------

Co-authored-by: parity-processbot <>

* removed `frame_support::sp_tracing::try_init_simple();`

* Review fixes

* Removed `as u64`

---------

Co-authored-by: Roman Useinov <[email protected]>
@EgorPopelyaev EgorPopelyaev mentioned this pull request Feb 27, 2023
Merged
EgorPopelyaev pushed a commit that referenced this pull request Feb 27, 2023
@bkontur @ruseinov @EgorPopelyaev
Tests + backport fix for teleport_assets (ReceiveTeleportedAsset) (#2235
a55d694 
)

* Wwstmint test for ReceiveTeleportedAsset

* Missing fix for `weigh_multi_assets`

* Added tests for statemine/statemint

* [Enhancement] Use XCM V3 for initiate_teleport weight calc (#2102)

* [Enhancement] Use XCM V3 for initiate_teleport weight calc

* deref

* replicate in all the runtimes

* fmt

* better handling for AllOf

* fmt

* small type fix

* replicate the fix for all runtimes

---------

Co-authored-by: parity-processbot <>

* removed `frame_support::sp_tracing::try_init_simple();`

* Review fixes

* Removed `as u64`

---------

Co-authored-by: Roman Useinov <[email protected]>
@EgorPopelyaev EgorPopelyaev mentioned this pull request Feb 27, 2023
Merged
EgorPopelyaev added a commit that referenced this pull request Feb 27, 2023
@EgorPopelyaev @bkontur @ruseinov
Tests + backport fix for teleport_assets (ReceiveTeleportedAsset) (#2235
9131b55 
) (#2239)

* Wwstmint test for ReceiveTeleportedAsset

* Missing fix for `weigh_multi_assets`

* Added tests for statemine/statemint

* [Enhancement] Use XCM V3 for initiate_teleport weight calc (#2102)

* [Enhancement] Use XCM V3 for initiate_teleport weight calc

* deref

* replicate in all the runtimes

* fmt

* better handling for AllOf

* fmt

* small type fix

* replicate the fix for all runtimes

---------

Co-authored-by: parity-processbot <>

* removed `frame_support::sp_tracing::try_init_simple();`

* Review fixes

* Removed `as u64`

---------

Co-authored-by: Branislav Kontur <[email protected]>
Co-authored-by: Roman Useinov <[email protected]>
EgorPopelyaev added a commit that referenced this pull request Feb 27, 2023
@EgorPopelyaev @bkontur @ruseinov
Tests + backport fix for teleport_assets (ReceiveTeleportedAsset) (#2235
75deb98 
) (#2240)

* Wwstmint test for ReceiveTeleportedAsset

* Missing fix for `weigh_multi_assets`

* Added tests for statemine/statemint

* [Enhancement] Use XCM V3 for initiate_teleport weight calc (#2102)

* [Enhancement] Use XCM V3 for initiate_teleport weight calc

* deref

* replicate in all the runtimes

* fmt

* better handling for AllOf

* fmt

* small type fix

* replicate the fix for all runtimes

---------

Co-authored-by: parity-processbot <>

* removed `frame_support::sp_tracing::try_init_simple();`

* Review fixes

* Removed `as u64`

---------

Co-authored-by: Branislav Kontur <[email protected]>
Co-authored-by: Roman Useinov <[email protected]>
paritytech-processbot bot pushed a commit that referenced this pull request Feb 27, 2023
@EgorPopelyaev @bkontur @ruseinov
Tests + backport fix for teleport_assets (ReceiveTeleportedAsset) (#2235
0ce339b 
) (#2238)

* Wwstmint test for ReceiveTeleportedAsset

* Missing fix for `weigh_multi_assets`

* Added tests for statemine/statemint

* [Enhancement] Use XCM V3 for initiate_teleport weight calc (#2102)

* [Enhancement] Use XCM V3 for initiate_teleport weight calc

* deref

* replicate in all the runtimes

* fmt

* better handling for AllOf

* fmt

* small type fix

* replicate the fix for all runtimes

---------

Co-authored-by: parity-processbot <>

* removed `frame_support::sp_tracing::try_init_simple();`

* Review fixes

* Removed `as u64`

---------

Co-authored-by: Branislav Kontur <[email protected]>
Co-authored-by: Roman Useinov <[email protected]>
@gilescope gilescope mentioned this pull request Mar 28, 2023
Closed
EgorPopelyaev added a commit that referenced this pull request Mar 28, 2023
@EgorPopelyaev
Revert "[Enhancement] Use XCM V3 for initiate_teleport weight calc (#…
83dbea9 
…2102)"

This reverts commit 8230ec4.
@EgorPopelyaev EgorPopelyaev mentioned this pull request Mar 28, 2023
Merged
gilescope added a commit that referenced this pull request Mar 29, 2023
@EgorPopelyaev @gilescope
[Revert] Use XCM V3 for initiate_teleport weight calc #2102 (#2394)
e98ed6e 
* Revert "[Enhancement] Use XCM V3 for initiate_teleport weight calc (#2102)"

This reverts commit 8230ec4.

* updating weight format

* We expect to pay 1bn+ for a teleport at the current weights.

* The test isn't needed and hardcoded scale is hand to maintain.

* remove unused imports

---------

Co-authored-by: Giles Cope <[email protected]>
EgorPopelyaev added a commit that referenced this pull request Mar 29, 2023
@EgorPopelyaev @gilescope
[Revert] Use XCM V3 for initiate_teleport weight calc #2102 (#2394)
9a722ba 
* Revert "[Enhancement] Use XCM V3 for initiate_teleport weight calc (#2102)"

This reverts commit 8230ec4.

* updating weight format

* We expect to pay 1bn+ for a teleport at the current weights.

* The test isn't needed and hardcoded scale is hand to maintain.

* remove unused imports

---------

Co-authored-by: Giles Cope <[email protected]>
EgorPopelyaev added a commit that referenced this pull request Mar 29, 2023
@EgorPopelyaev @gilescope
[Revert] Use XCM V3 for initiate_teleport weight calc #2102 (#2394)
ce2d535 
* Revert "[Enhancement] Use XCM V3 for initiate_teleport weight calc (#2102)"

This reverts commit 8230ec4.

* updating weight format

* We expect to pay 1bn+ for a teleport at the current weights.

* The test isn't needed and hardcoded scale is hand to maintain.

* remove unused imports

---------

Co-authored-by: Giles Cope <[email protected]>
EgorPopelyaev added a commit that referenced this pull request Mar 30, 2023
@EgorPopelyaev @gilescope
[Revert] Use XCM V3 for initiate_teleport weight calc #2102 (#2394) (#…
269fb07 
…2402)

* Revert "[Enhancement] Use XCM V3 for initiate_teleport weight calc (#2102)"

This reverts commit 8230ec4.

* updating weight format

* We expect to pay 1bn+ for a teleport at the current weights.

* The test isn't needed and hardcoded scale is hand to maintain.

* remove unused imports

---------

Co-authored-by: Giles Cope <[email protected]>
EgorPopelyaev added a commit that referenced this pull request Mar 30, 2023
@EgorPopelyaev @gilescope
[Revert] Use XCM V3 for initiate_teleport weight calc #2102 (#2394) (#…
e05c8d7 
…2403)

* Revert "[Enhancement] Use XCM V3 for initiate_teleport weight calc (#2102)"

This reverts commit 8230ec4.

* updating weight format

* We expect to pay 1bn+ for a teleport at the current weights.

* The test isn't needed and hardcoded scale is hand to maintain.

* remove unused imports

---------

Co-authored-by: Giles Cope <[email protected]>
@kiltbot kiltbot mentioned this pull request Apr 3, 2023
Closed
@librelois librelois mentioned this pull request Apr 6, 2023
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@KiChjang KiChjang KiChjang left review comments

@gavofyork gavofyork gavofyork approved these changes

@gilescope gilescope gilescope approved these changes

@ggwpez ggwpez Awaiting requested review from ggwpez

@joepetrowski joepetrowski Awaiting requested review from joepetrowski

Assignees

@ruseinov ruseinov

Labels
A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes 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. T1-runtime This PR/Issue is related to the topic “runtime”. T6-XCM This PR/Issue is related to XCM.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Fix] Hardcoded weight for initiate_teleport
4 participants
@ruseinov @gavofyork @gilescope @KiChjang