Skip to content
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.

Sign up for GitHub

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

Jump to bottom

asset-rate pallet: box asset kind parameter #1545

Merged
merged 8 commits into from
Sep 15, 2023
Merged

asset-rate pallet: box asset kind parameter #1545

merged 8 commits into from
Sep 15, 2023

Conversation

muharem
Copy link
Contributor

@muharem muharem commented Sep 13, 2023 •
edited
Loading

The AssetKind type parameter of a dispatchable, defined by the user, might be large — like xcm::MultiLocation. To prevent inflating the size of the Call type, we Box it.

This changes required for #1333

@muharem muharem added T1-FRAME This PR/Issue is related to core FRAME, the framework. T2-pallets This PR/Issue is related to a particular pallet. labels Sep 13, 2023
@muharem muharem marked this pull request as ready for review September 14, 2023 09:39
@muharem muharem requested review from a team and removed request for a team September 14, 2023 09:39
ggwpez
ggwpez approved these changes Sep 14, 2023
View reviewed changes
Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

There are some un-needed clones, but looking good in general.

substrate/frame/asset-rate/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/asset-rate/src/lib.rs Outdated Show resolved Hide resolved
@muharem muharem requested a review from ggwpez September 14, 2023 10:41
@paritytech-ci paritytech-ci requested review from a team September 14, 2023 10:49
@muharem
Copy link
Contributor Author

muharem commented Sep 14, 2023 •
edited
Loading

@ggwpez can you look again please.
I also updated traits ConversionFromAssetBalance and ConversionToAssetBalance accept AssetId as ref to avoid extra clone. I will update the PR title if looks good to you.

@muharem
Copy link
Contributor Author

muharem commented Sep 14, 2023

probably better to have separate PR for the traits

@ggwpez
Copy link
Member

ggwpez commented Sep 14, 2023

@ggwpez can you look again please. I also updated traits ConversionFromAssetBalance and ConversionToAssetBalance accept AssetId as ref to avoid extra clone. I will update the PR title if looks good to you.

Historically the converters use instances and not references, eg like in the Convert trait.
I think it makes sense to keep that, since the functions are called to_asset_balance, which indicated consumption of the argument (as compared to as_asset_balance).

@muharem
Copy link
Contributor Author

muharem commented Sep 14, 2023

@ggwpez reverted

@muharem
Copy link
Contributor Author

muharem commented Sep 14, 2023

@ggwpez can you reapprove if looks good, I did reset the previous approve from you.

@muharem muharem mentioned this pull request Sep 15, 2023
Merged
@muharem muharem merged commit 841a33e into master Sep 15, 2023
@muharem muharem deleted the muharem-asset-rate-box-parameter branch September 15, 2023 09:52
@muharem muharem added the T4-runtime_API This PR/Issue is related to runtime APIs. label Sep 18, 2023
@kiltbot kiltbot mentioned this pull request Oct 19, 2023
Closed
@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/polkadot-release-analysis-v1-2-0/4451/1

@ahmadkaouk ahmadkaouk mentioned this pull request Nov 16, 2023
Closed
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
@muharem @ggwpez
asset-rate pallet: box asset kind parameter (paritytech#1545)
66c9911 
The `AssetKind` type parameter of a dispatchable, defined by the user,
might be large — like `xcm::MultiLocation`. To prevent inflating the
size of the `Call` type, we `Box` it.

This changes required for
paritytech#1333

---------

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
bkchr pushed a commit that referenced this pull request Apr 10, 2024
@serban300 @bkchr
Call To*InboundLaneApi::message_details() with batched messages (#1545)
050b12f 
* generated_message_details() -> Simplifications

- avoid using a HashMap for `messages_to_refine`. It seems that a vec is
  enough
- minimize the number of conversions between `OutboundMessageDetails` and
  `MessageDetails`
- use references where possible in order to minimize the number of
  intermediary Vecs
- simplify `make_message_details_map()` logic, reduce its scope and rename
  it to `validate_out_msgs_details()`

Signed-off-by: Serban Iorga <serban@parity.io>

* Define typed_state_call()

Signed-off-by: Serban Iorga <serban@parity.io>

* Call To*InboundLaneApi::message_details() with single messages

Signed-off-by: Serban Iorga <serban@parity.io>

* Call To*InboundLaneApi::message_details() with batched messages

Signed-off-by: Serban Iorga <serban@parity.io>

* validate_out_msgs_details() -> change check

* Define split_msgs_to_refine()

Signed-off-by: Serban Iorga <serban@parity.io>

Signed-off-by: Serban Iorga <serban@parity.io>
This was referenced Jun 5, 2024
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ggwpez ggwpez ggwpez approved these changes

@juangirini juangirini juangirini approved these changes

Assignees
No one assigned
Labels
T1-FRAME This PR/Issue is related to core FRAME, the framework. T2-pallets This PR/Issue is related to a particular pallet. T4-runtime_API This PR/Issue is related to runtime APIs.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@muharem @ggwpez @Polkadot-Forum @juangirini