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

Extend Consideration trait #1813

Closed
wants to merge 2 commits into from
Closed

Extend Consideration trait #1813

wants to merge 2 commits into from

Conversation

NachoPal
Copy link
Contributor

@NachoPal NachoPal commented Oct 7, 2023 •
edited
Loading

Draft exploring the idea of extending Consideration trait with new_from_exact(who: &AccountId, new: Balance)

It would be useful for migrations from Deposits to Consideration tickets where a fixed Footprint might not always return the expected deposited Balance.

Is it possible to do it in a different way? right now it creates a dependency on Currency, which I think is actually supposed to be removed in favor of Consideration.

@liamaharon
Copy link
Contributor

liamaharon commented Oct 7, 2023 •
edited
Loading

Is it possible to do it in a different way? right now it creates a dependancy on Currency, which I think is actually supposed to be removed in favor of Consideration.

I don't see any dependency on Currency being introduced here?

This approach seems fine to me.

@NachoPal
Copy link
Contributor Author

NachoPal commented Oct 8, 2023 •
edited
Loading

I don't see any dependency on Currency being introduced here?

The dependency is introduced when using BalanceOf<T>.

See #1569 (Edited, it wasn't linking to the right place)

@liamaharon
Copy link
Contributor

liamaharon commented Oct 10, 2023 •
edited
Loading

The dev only needs to specify the Balance type, this type is not bound to Currency.

All implementations of Consideration already assume the existence of a Balance, so this feels fine to me.

@NachoPal
Copy link
Contributor Author

bot fmt

@command-bot
Copy link

command-bot bot commented Oct 10, 2023 •
edited
Loading

@NachoPal https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3922343 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 15-565c5f0f-0872-4b29-ba37-b37dfcb211c9 to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Oct 10, 2023

@NachoPal Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3922343 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3922343/artifacts/download.

@NachoPal NachoPal added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Oct 10, 2023
@NachoPal NachoPal marked this pull request as ready for review October 10, 2023 12:44
@NachoPal NachoPal requested review from a team, liamaharon and kianenigma October 10, 2023 12:44
@NachoPal NachoPal requested review from gpestana, Ank4n and ggwpez October 16, 2023 08:57
franciscoaguirre
franciscoaguirre reviewed Nov 2, 2023
View reviewed changes
substrate/frame/support/src/traits/storage.rs
Comment on lines +216 to +219
/// Create a ticket for a `new` balance attributable to `who`. This ticket *must* ultimately
/// be consumed through `update` or `drop` once the footprint changes or is removed.
/// This is useful when a new ticket needs to be created with a precise balance, instead of
/// deriving it from a footprint
Copy link
Contributor

Choose a reason for hiding this comment

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

The doc comment still mentions a footprint

kianenigma
kianenigma reviewed Nov 2, 2023
View reviewed changes
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

High level, one of the main goals of Consideration is to abstract away the need for the existence of any "Balance/Currency" sub-system in a runtime, the fact that we need to add a Balance generic to the trait now seems like the wrong compromise to me.

I recall this was solving an important issue with migrations though, so I would not block it. Possibly it can be a temporary hack, in which case you can document the method as "it is temporary".

What alternatives we have?

@juangirini
Copy link
Contributor

Which migrations would benefit from this?

@NachoPal
Copy link
Contributor Author

NachoPal commented Nov 2, 2023 •
edited
Loading

Which migrations would benefit from this?

@juangirini Those were the Deposit might not have been constant over time (increased by a runtime upgrade).

During the migration, the new HoldConsideration will result in a fixed Balance (based on a fixed Footprint and a constant Convert function).

Let's say that during the migration, the new HoldConsideration is expected to have Balance = 10. What would happen when for a particular account the stored Deposit = 5? After Currency::unreserve(account, 5), when trying to create a new ticket to hold Balance = 10, it might fail (not enough balance). Currently, HoldConsideration::new() returns either Ok(()) or Err. What to do if it is not possible to hold the balance?

What alternatives we have?

@kianenigma The only alternative I see is to waive the deposit and not generate any ticket.

@ggwpez
Copy link
Member

ggwpez commented Nov 2, 2023 •
edited
Loading

After discussing it I think it would be less invasive to create a type solely for migrating from Currency.
Something like HoldConsiderationFromLegacy or whatever that has this new_from_exact function (plus a trait for it).
That way we dont have to modify the normal production version of Hold / FreezeConsiderations.

gavofyork
gavofyork reviewed Nov 2, 2023
View reviewed changes
substrate/frame/support/src/traits/storage.rs
@@ -208,11 +208,17 @@ where
/// treated as one*. Don't type to duplicate it, and remember to drop it when you're done with
/// it.
#[must_use]
pub trait Consideration<AccountId>: Member + FullCodec + TypeInfo + MaxEncodedLen {
pub trait Consideration<AccountId, Balance>: Member + FullCodec + TypeInfo + MaxEncodedLen {
Copy link
Member

Choose a reason for hiding this comment

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

Gah! No!

Copy link
Member

Choose a reason for hiding this comment

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

Considerations need not have anything to do with balances.

@NachoPal NachoPal closed this Nov 2, 2023
@NachoPal
Copy link
Contributor Author

NachoPal commented Nov 2, 2023

Closing it. I'll give it a try to this alternative: #1813

@NachoPal NachoPal mentioned this pull request Nov 10, 2023
Open
bkchr pushed a commit that referenced this pull request Apr 10, 2024
@bkontur @bkchr
Backport xcm bridging extensions to the bridge repo (#1813)
a1c6502 
* Backport xcm bridging extensions to the bridge repo

* Fixes

* Fixes
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

@kianenigma kianenigma kianenigma left review comments

@franciscoaguirre franciscoaguirre franciscoaguirre left review comments

@gavofyork gavofyork gavofyork left review comments

@liamaharon liamaharon liamaharon approved these changes

@gpestana gpestana Awaiting requested review from gpestana

@Ank4n Ank4n Awaiting requested review from Ank4n

@ggwpez ggwpez Awaiting requested review from ggwpez

Assignees
No one assigned
Labels
T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@NachoPal @liamaharon @juangirini @ggwpez @gavofyork @franciscoaguirre @kianenigma