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.

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

Frame: Consideration trait generic over Footprint and indicates zero cost #4596

Merged
merged 7 commits into from
Jun 22, 2024

Conversation

muharem
Copy link
Contributor

@muharem muharem commented May 27, 2024

Consideration trait generic over Footprint and indicates zero cost for a give footprint.

Consideration trait is generic over Footprint (currently defined over the type with the same name). This makes it possible to setup a custom footprint (e.g. current number of proposals in the storage).

Consideration::new and Consideration::update return an Option<Self> instead Self, this make it possible to indicate a no cost for a specific footprint (e.g. if current number of proposals in the storage < max_proposal_count / 2 then no cost).

These cases need to be handled for #3151

@muharem muharem added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label May 27, 2024
@muharem muharem requested a review from a team as a code owner May 27, 2024 11:08
fn new(who: &AccountId, new: Footprint) -> Result<Self, DispatchError>;
/// be consumed through `update` or `drop` once the footprint changes or is removed. `None`
/// implies no cost for a given footprint.
fn new(who: &AccountId, new: Footprint) -> Result<Option<Self>, DispatchError>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Self can also say for itself that there is no cost (with 0 balance for example), but this wont let a pallet to have no storage entry for such case.

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable 1/3
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6433841

@@ -122,7 +122,9 @@ pub mod pallet {
type ManagerOrigin: EnsureOrigin<Self::RuntimeOrigin>;

/// A means of providing some cost while data is stored on-chain.
type Consideration: Consideration<Self::AccountId>;
///
/// Should never return a `None`, implying no cost for a non-empty preimage.
Copy link
Member

Choose a reason for hiding this comment

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

Why should it not do that? The implementor of the Consideration should be able to return as they see fit.
Otherwise it would probably need a Consideration and MaybeConsideration trait to disambiguate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree. But I did now want to change the behaviour of this pallet, and make only a patch. Otherwise we need a migration for the preimage pallet.
Maybe I should rephrase it, that we do expect some cost for any non zero footprint, so it should not return none.

Copy link
Member

Choose a reason for hiding this comment

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

Okay i see. The maybe add a check to the integrity_test of the pallet that a zero sized preimage still needs a deposit.

@muharem muharem changed the title Frame: Consideration trait generic over Footprint and handles zero cost Frame: Consideration trait generic over Footprint and indicates zero cost Jun 17, 2024
@muharem muharem added this pull request to the merge queue Jun 22, 2024
Merged via the queue into master with commit 812dbff Jun 22, 2024
159 of 161 checks passed
@muharem muharem deleted the muharem-generic-consideration branch June 22, 2024 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants