Introduces account existence providers reference counting#7363
Introduces account existence providers reference counting#7363
Conversation
gavofyork
added
A3-in_progress
| /// An abstraction of a value stored within storage, but possibly as part of a larger composite | ||
| /// item. | ||
| pub trait StoredMap<K, T> { | ||
| pub trait StoredMap<K, T: Default> { |
There was a problem hiding this comment.
Is it really necessary to have T: Default?
There was a problem hiding this comment.
Basically there must be a way to determine whether the value is set or not, since the account item can now exist without the data being set (via some external existential provider, unlike before). Two other options are to make it require an IsProvided trait or to store the data under an Option. The latter would require an additional migration (😕). And I generally try to avoid introducing extra traits.
There was a problem hiding this comment.
afaik, change storage from non option type to option type does not require migration. Option wrapping is handled by the decl_storage macro.
There was a problem hiding this comment.
+1 that option should not change the underlying storage
There was a problem hiding this comment.
The option would not be for a storage item itself, but rather an inner component of a storage item's structure. Concretely, it would be the data field of the frame_system::AccountInfo (i.e. T::AccountData) which would need to become an Option, which would require a migration.
| /// Maybe mutate the item only if an `Ok` value is returned from `f`. Do nothing if an `Err` is | ||
| /// returned. It is removed or reset to default value if it has been mutated to `None` | ||
| fn try_mutate_exists<R, E>(k: &K, f: impl FnOnce(&mut Option<T>) -> Result<R, E>) -> Result<R, E>; | ||
| fn try_mutate_exists<R, E: From<StoredMapError>>( |
There was a problem hiding this comment.
Maybe should add associated type type Error to avoid hard dependency on StoredMapError here? Otherwise it feels bit too coupled with the frame system implementation.
There was a problem hiding this comment.
No - need to be like this otherwise it cannot support this error assimilation API.
|
|
||
| /// True if we have upgraded so that AccountInfo contains two types of `RefCount`. False | ||
| /// (default) if not. | ||
| UpgradedToDualRefCount build(|_| true): bool; |
There was a problem hiding this comment.
Note: you can use the pallet storage version now.
There was a problem hiding this comment.
IMHO best would be to have an isolated function which does this migration, and we can also do a version bump.
the isolated function wouldn't depend on system so that there is no burden to maintain it.
I understand it is less handy for polkadot so I'm ok to have it as it is now. But from a substrate perspective having an isolated code anyone can pickup is better, than a inline migration which will be removed after polkadot runtime upgrades, or removed when it start being annoying to maintain.
There was a problem hiding this comment.
Isolated function is done.
…strate into gav-account-provider-refs
|
Any updates on this? We would love this use this feature as soon as possible. |
gavofyork
added
A0-please_review
Co-authored-by: Alexander Popiak <alexander.popiak@parity.io>
Co-authored-by: Alexander Popiak <alexander.popiak@parity.io>
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
kianenigma
left a comment
kianenigma
left a comment
There was a problem hiding this comment.
Had one last brief look and LGTM.
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
s3krit
added
the
D1-audited 👍
9 participants
Fixes #7343
Polkadot companion: paritytech/polkadot#2152