refactor: migrate runtime storage Vec, BTreeSet, BTreeMap to their bounded versions#228
refactor: migrate runtime storage Vec, BTreeSet, BTreeMap to their bounded versions#228
Conversation
fix: clippy
* feat: initial commit * feat: pallet compiling * wip: fixing test cases * wip: refactoring test cases * test: unit tests passing again * benchmark tests passing * test: attestation unit tests passing * chore: clippy fixes * chore: fmt fixes * bench: re-execute benchmarks for delegation and attestation + temporarily remove benchmarks from peregrine runtime * fix: adjust pre-condition for an attestation unit test * wip: add initial support for delegation storage migrations * delegation storage migrator compiling * wip: storage migrator working, now writing unit tests for it * test: unit tests passing * chore: clippy hints * chore: further clippy fixes * chore: fmt * wip: adjusting last things before possible to review * chore: clippy + fmt * fix: address part of PR review comments * fix: more comments from PR review * wip: change vector of versions to enum * test: unit tests refactor for the new storage migrator version * chore: refactor * test: unit tests passing again * chore: move deprecated type to deprecated.rs * chore: refactor import/export * doc: refine documentation * chore: fmt + clippy * test: add TODO to improve post_migration tests * feat: remove test feature from traits * test: add checks during migration in try-runtime * bench: add spiritnet benchmarks for delegation and attestation * feat: additional logs for count of migrated nodes * chore: fmt * chore: removed useless file * chore: bump runtime version * fix: PR comments * fix: re-introduce root_id in delegation creation hash * chore: capitalize storage version enum * chore: move migrations in their own module * chore: move migrations.rs out of the migrations folder * fix: add check for parent revocation status when creating a delegation * chore: fmt * feat: remove revoke_hierarchy extrinsic * bench: add peregrine benchmarks * bench: re-run delegation and attestation default benchmarks * chore: fmt
There was a problem hiding this comment.
Since this PR has grown more than I expected and includes a refactor in DID which should have been handled in a separate PR, I have left comments in a few places to give some background information which hopefully make your review process much easier.
This PR is low-priority but must be merged before we submit our wasm blob.
Enjoy @weichweich @Diiaablo95 🥲
| pub const MaxSignatureByteLength: u16 = 64; | ||
| pub const MaxParentChecks: u32 = 5; | ||
| pub const MaxRevocations: u32 = 5; | ||
| #[derive(Clone)] |
There was a problem hiding this comment.
I feel like deriving Debug, Clone, Ord or PartialEq should not be required for u32. Yet, the compiler complained if the corresponding associated types defined in the Config required to implement these traits and I did not derive them in the const declarations. How can this be improved @weichweich ?
You will notice a bunch of these more below. I just commented here because this is the first occasion.
There was a problem hiding this comment.
interesting. The thing here is that this is not a "normal" const but a new type called MaxChildren and that MaxChildren doesn't implement the clone trait!?
There was a problem hiding this comment.
Yes. But why though? Shouldn't this be implemented by default if the type is a u32?
pallets/parachain-staking/src/lib.rs
Outdated
|
|
||
| // attempt to insert candidate and check for excess | ||
| let insert_candidate = candidates | ||
| .insert(Stake { |
There was a problem hiding this comment.
Note: Throws if insertion would exceed the bounded vec's max size. Returns true if the item is unique in the set, otherwise returns false.
We could probably remove the upper check for TooManyCollatorCandidates?
There was a problem hiding this comment.
yep i think we should remove the redundant check
|
I'll check it out tomorrow. I am going to open another one soon for the service endpoints stuff. And then I guess we can merge this one. |
Sure, we definitely need to finish and merge your stuff first. Still hope you can do at least a light review as I am touching a lot of your DID code. |
wischli
changed the title
weichweich
left a comment
weichweich
left a comment
There was a problem hiding this comment.
Nice! I think we should look into the StorageInfo thing and maybe add that to the pallet.
| pub const MaxSignatureByteLength: u16 = 64; | ||
| pub const MaxParentChecks: u32 = 5; | ||
| pub const MaxRevocations: u32 = 5; | ||
| #[derive(Clone)] |
There was a problem hiding this comment.
interesting. The thing here is that this is not a "normal" const but a new type called MaxChildren and that MaxChildren doesn't implement the clone trait!?
pallets/did/src/did_details.rs
Outdated
| pub did: DidIdentifierOf<T>, | ||
| /// The new key agreement keys. | ||
| pub new_key_agreement_keys: BTreeSet<DidEncryptionKey>, | ||
| pub new_key_agreement_keys: BoundedBTreeSet<DidEncryptionKey, T::MaxTotalKeyAgreementKeys>, |
There was a problem hiding this comment.
Maybe we could add a type alias for BoundedBTreeSet<DidEncryptionKey, T::MaxTotalKeyAgreementKeys> and similar constructs?
There was a problem hiding this comment.
Applied in a807d3f. However, I am not happy with the naming yet.
ntn-x2
left a comment
ntn-x2
left a comment
There was a problem hiding this comment.
Ok, I guess there has been some confusion regarding some configuration parameters in the DID pallet. My bad if the reason was poor documentation. I tried to address all the inconsistencies by now. I definitely love the refactoring of mocking 👍 Good job, but one more sprint to do after we merge #235 😁
|
See this comment regarding the many |
3 participants
fixes KILTProtocol/ticket#1288
{Vec | BTreeSet | BTreeMap}runtime storages toBounded*which introduces a hard check to the maximum length of the vector upon insertionparachain-stakingbecause I thought we would need to do runtime upgrades for the migration. However, it turned out that we do not need to do those because we enforced the bounds before. This does not hold true forMaxDelegatedAttestationsand new bounds in the DID pallet. However, I suppose we will eventually pick numbers which will be much high and not result in a conflict with any potential item stored on the chain right now.tests.rsandbenchmarking.rs.Please note that
Bounded*does not implementDerefMuton purpose because it would allow for unchecked extension of the inner vector. This mostly impacts (re-) sorting of the inner vector inparachain-stakingbecause we have to sort the clone of the innerVecand then create a newBoundedVecfrom that.ToDo
custom types
Types companion
This PR introduces new custom JS-types which are required for compatibility with our SDK and the Polkadot Apps. Please use the following types to test the code with the Polkadot Apps:
JS-Types
{ "DidFragmentUpdateAction_ServiceEndpoints": { "_enum": { "Ignore": "Null", "Change": "ServiceEndpoints", "Delete": "Null" } }, "DidFragmentUpdateAction_DidVerificationKey": { "_enum": { "Ignore": "Null", "Change": "DidVerificationKey", "Delete": "Null" } }, "ContentType": { "_enum": [ "ApplicationJson", "ApplicationJsonLd" ] }, "DidStorageVersion": { "_enum": [ "V1", "V2" ] }, "OrderedSet": "BoundedVec<Stake, MaxCollatorCandidates>", "MaxCollatorCandidates": "u32", "Collator": { "id": "AccountId", "stake": "Balance", "delegators": "OrderedSet<Stake, MaxDelegatorsPerCollator", "total": "Balance", "state": "CollatorStatus" }, "MaxDelegatorsPerCollator": "u32", "Delegator": { "delegations": "OrderedSet<Stake, MaxCollatorsPerDelegator", "total": "Balance" }, "MaxCollatorsPerDelegator": "u32", "MaxDelegatedAttestations": "u32", "MaxClaims": "u32", "DelegationNode": { "hierarchyRootId": "DelegationNodeIdOf", "parent": "Option<DelegationNodeIdOf>", "children": "BoundedBTreeSet<DelegationNodeIdOf, MaxChildren>", "details": "DelegationDetails" }, "MaxChildren": "u32", "DidNewKeyAgreementKeys": "BoundedBTreeSet<DidEncryptionKey, MaxNewKeyAgreementKeys>", "DidKeyAgreementKeys": "BoundedBTreeSet<KeyIdOf, MaxTotalKeyAgreementKeys>", "DidVerificationKeysToRevoke": "BoundedBTreeSet<KeyIdOf, MaxVerificationKeysToRevoke>", "MaxNewKeyAgreementKeys": "u32", "MaxTotalKeyAgreementKeys": "u32", "MaxVerificationKeysToRevoke": "u32", "MaxPublicKeysPerDid": "u32", "DidPublicKeyMap": "BoundedBTreeMap<KeyIdOf, DidPublicKeyDetails, MaxPublicKeysPerDid>", "DidCreationDetails": { "did": "DidIdentifierOf", "newKeyAgreementKeys": "DidNewKeyAgreementKeys", "newAttestationKey": "Option<DidVerificationKey>", "newDelegationKey": "Option<DidVerificationKey>", "newServiceEndpoints": "Option<ServiceEndpoints>" }, "DidUpdateDetails": { "newAuthenticationKey": "Option<DidVerificationKey>", "newKeyAgreementKeys": "DidNewKeyAgreementKeys", "attestationKeyUpdate": "DidFragmentUpdateAction_DidVerificationKey", "delegationKeyUpdate": "DidFragmentUpdateAction_DidVerificationKey", "publicKeysToRemove": "DidVerificationKeysToRevoke", "serviceEndpointsUpdate": "DidFragmentUpdateAction_ServiceEndpoints" }, "DidDetails": { "authenticationKey": "KeyIdOf", "keyAgreementKeys": "DidKeyAgreementKeys", "delegationKey": "Option<KeyIdOf>", "attestationKey": "Option<KeyIdOf>", "publicKeys": "DidPublicKeyMap", "serviceEndpoints": "Option<ServiceEndpoints>", "lastTxCounter": "u64" }, "ServiceEndpoints": { "contentHash": "Hash", "urls": "BoundedVec<Url, MaxEndpointUrlsCount>", "contentType": "ContentType" }, "HttpUrl": { "payload": "BoundedVec<u8, MaxUrlLength>" }, "FtpUrl": { "payload": "BoundedVec<u8, MaxUrlLength>" }, "IpfsUrl": { "payload": "BoundedVec<u8, MaxUrlLength>" }, "MaxUrlLength": "u32", "MaxEndpointUrlsCount": "u32", "StorageError": { "_enum": { "DidAlreadyPresent": "Null", "DidNotPresent": "Null", "DidKeyNotPresent": "DidVerificationKeyRelationship", "VerificationKeyNotPresent": "Null", "CurrentlyActiveKey": "Null", "MaxTxCounterValue": "Null", "MaxPublicKeysPerDidKeyIdentifierExceeded": "Null", "MaxTotalKeyAgreementKeysExceeded": "Null", "MaxOldAttestationKeysExceeded": "Null" } } }Checklist:
array[3]useget(3), ...)