feat: add deposits for DIP provider pallet #574

ntn-x2 · 2023-11-02T12:20:10Z

Fixes https://github.com/KILTprotocol/ticket/issues/2983.

Add a generic pallet to store deposits, as well as a hooking mechanism to both the existing provider pallet (to allow runtimes that take deposits to take deposits, or to do any other operation), and to the new deposit pallet, which allows other pallets to clean up storage if a storage is reclaimed back by its owner (e.g., an identity commitment is removed).

Tests will follow in a separate PR, when time allows for them 😁 The whole DIP branch won't be merged on develop before tests are written, so it's not a risk. The logic has been tested by spinning up a local network and trying to commit an identity (which takes a deposit), remove the commitment (which frees up the deposit), re-commit the identity, reclaim the deposit (which removes the commitment).

Implement logic for deposit removal hooks
Test
Review

dip-template/runtimes/dip-provider/src/dip.rs

Ad96el

Very nice! I like the architectural design and the default implementation of the traits. Some comments are a bit nitpicking - sorry for that. I assume the pallet-deposit-storage will also be deployed on Spiritnet, right? If so, the storage version has to be initialized in the runtimes. Additionally, I would appreciate more documentation. Perhaps this can be addressed in another PR?

pallets/pallet-deposit-storage/src/lib.rs

pallets/pallet-dip-provider/src/lib.rs

dip-template/runtimes/dip-provider/Cargo.toml

dip-template/runtimes/dip-provider/src/lib.rs

Ad96el

🚀

pallets/pallet-dip-provider/src/lib.rs

Ad96el · 2023-11-07T08:41:31Z

pallets/pallet-dip-provider/src/lib.rs


+	pub type IdentityProviderOf<T> = <T as Config>::IdentityProvider;
 	pub type IdentityOf<T> = <<T as Config>::IdentityProvider as IdentityProvider<<T as Config>::Identifier>>::Success;
 	pub type IdentityCommitmentVersion = u16;


For my learning: Why not create a type in the config that implements the default trait? Wouldn't it be cleaner? I often struggle to determine what should be included in the configuration and what should not. 😕

You mean using the latest Substrate features that allow to specify defaults for a pallet's Config trait? Or what do you mean?

I took a shot in #577. Resolving this, let me know there what you think!

trusch · 2023-11-08T14:00:13Z

pallets/pallet-deposit-storage/src/deposit.rs

+	}
+}
+
+impl<Runtime, DepositsNamespace, FixedDepositAmount> DipProviderHooks<Runtime>


Somehow it feels weird to see this tight coupling of the deposits and the dip pallet here. Shouldn't this logic also be passed in while configuring the pallet for a specific runtime like with the removal?

Do you think the name of the type is misleading? This is already a concrete implementation of a trait of the DIP provider pallet, and it is supposed to work only when you are storing identity commitments and want to take deposits for them. I don't understand what you refer with the logic being passed? Can you elaborate on that?

I think the question is more from an architectural point of view: Why does something that is obviously specific to the DIP pallet (like this implementation of the DipProviderHooks) living within the deposit-storage pallet? Consequently this would mean that the deposit pallet code base would need to grow with every new "consumer" pallet that uses the deposit pallet. I think no action item should result from this comment, I'm only interested in your reasoning for structuring it this way.

The problem is the Rust orphan rule. You can only implement a trait for a type if either of them is native to the crate. In this case, it would mean either implementing the trait in the pallet storage or in the pallet provider crate, as I see no point in having it in any other place. Would you rather have the type in the provider crate?

trusch

lgtm in general, just one question/remark

Fixes KILTprotocol/ticket#2974 and based on top of #574. This is my take at trying to improve the DX of integrating DIP components. The following is a list of the main changes introduced by this PR: - Definitions of traits that are only ever used and useful inside a pallet have been changed to take a generic `Runtime` type that must implement the pallet config, reducing the clutter when implementing them - Additional bounds have been added to the associated types of those traits, so that they don't need to be specified in the pallet config so that we can have trait bounds of those types. So basically what used to be the trait bounds on the pallet config, is not a trait bound on the trait return type. I think this is also the "Rusty" way to go. - The generic consumer types exported in `kilt-dip-support` crate have been renamed with the `Generic` prefix, because I introduced two new types, starting with `Kilt`, that make it easier to integrate any KILT runtime as an identity provider, with the downside of having to depend on the whole runtime definition, but there's also ways around that (e.g., by having an intermediate crate that only implements the required `Config` traits). Example 1 below shows the change. - Better re-exports (feedback is welcome on this one). For instance, for types that implement a basic version of a pallet's trait definition, the type is exported directly from the pallet crate root, while the trait is still namespaced behind the `mod traits`. Example 2 below. - Change of the origin check for the `pallet_dip_consumer::dispatch_as` extrinsic from `ensure_signed` to a generic `T::DispatchOriginCheck::ensure_origin`, as long as the `DispatchOriginCheck` returns an `AccountId` upon successful verification. ## Example 1: old vs new way of integrating a consumer into a runtime where KILT is a provider ### Old ```rust pub type ProofVerifier = VersionedDipSiblingProviderStateProofVerifier< RelayStateRootsViaRelayStorePallet<Runtime>, ConstU32<2_000>, ProviderParachainStateInfoViaProviderPallet<ProviderRuntime>, AccountId, BlakeTwo256, KeyIdOf<ProviderRuntime>, ProviderAccountId, Web3Name, LinkableAccountId, 10, 10, u128, // Signatures are valid for 50 blocks FrameSystemDidSignatureContext<Runtime, 50>, DipCallFilter, >; ``` ### New (100% equivalent to the old one) ```rust pub type ProofVerifier = KiltVersionedSiblingProviderVerifier< ProviderRuntime, // KILT runtime definition ConstU32<2_000>, RelayStateRootsViaRelayStorePallet<Runtime>, // Local runtime definition BlakeTwo256, DipCallFilter, 10, 10, 50, >; ``` ## Example 2: example of type implementing a pallet's trait with a basic implementation For the trait ```rust pub trait IdentityCommitmentGenerator<Runtime> where Runtime: Config, Runtime::IdentityProvider: IdentityProvider<Runtime>, { type Error: Into<u16>; type Output: Clone + Eq + Debug + TypeInfo + FullCodec + MaxEncodedLen; fn generate_commitment( identifier: &Runtime::Identifier, identity: &IdentityOf<Runtime>, version: IdentityCommitmentVersion, ) -> Result<Self::Output, Self::Error>; } ``` a basic implementation (mostly useful for tests) is provided ```rust pub struct DefaultIdentityCommitmentGenerator<Output>(PhantomData<Output>); impl<Runtime, Output> IdentityCommitmentGenerator<Runtime> for DefaultIdentityCommitmentGenerator<Output> where Runtime: Config, Output: Default + Clone + Eq + Debug + TypeInfo + FullCodec + MaxEncodedLen, { type Error = u16; type Output = Output; fn generate_commitment( _identifier: &Runtime::Identifier, _identity: &IdentityOf<Runtime>, _version: IdentityCommitmentVersion, ) -> Result<Self::Output, Self::Error> { Ok(Output::default()) } } ``` The pallet's crate will then export the trait with `pub mod traits`, while the type is re-exported directly _also_ from the root with `pub use traits::DefaultIdentityCommitmentGenerator`.

Feature branch for everything DIP. It will collect other PRs until we are happy with the features, and will add the DIP to some of our runtimes and merge this into `develop`. ## WIP Checklist for the open tasks for v1 - [x] Basic structure -> #489 - [x] Merkleization of DID Documents -> #492 - [x] `RuntimeCall` verification logic -> #502 - [x] DID signature verification -> #516 - [x] Add support for linked accounts and web3name -> #525 - [x] Configurable origin for `commit_identity` -> #526 - [x] Proper fee management -> #528 - [x] Update to Polkadot 0.9.43 -> c18a6ce - [x] Replace XCM with state proofs -> #543 - [x] Add support for relaychain consumer -> #553 (part of #543) - [x] Proper error handling -> #572 - [x] Add support for versioning -> #573 - [x] Take deposits for identity commitments -> #574 - [x] Expose common definitions usable by consumers -> #577 - [x] Change ensure_signed! to configurable origin also for the `dispatch_as` function -> #577 - [x] Proper benchmarking and weights -> #585 - [x] Comments and docs -> #584 - [x] Revert Dockerfile changes in #587 - [x] [OPTIONAL] Add support for Zombienet -> #587 - [x] [OPTIONAL] Add chain spec loading from file for template runtimes -> #587 - [x] Big, final review -> #494 (review) - [x] Improvements n.1 PR -> #591 - [x] Improvements n.2 PR -> #592 - [x] Add to Peregrine runtime -> #594 - [ ] Deploy on Peregrine - [ ] Unit tests - [ ] Add to Spiritnet runtime - [ ] Deploy on Spiritnet - [ ] [OPTIONAL] Move DIP-related stuff into its own repo --------- Co-authored-by: Adel Golghalyani <[email protected]> Co-authored-by: Chris Chinchilla <[email protected]> Co-authored-by: Albrecht <[email protected]>

Feature branch for everything DIP. It will collect other PRs until we are happy with the features, and will add the DIP to some of our runtimes and merge this into `develop`. ## WIP Checklist for the open tasks for v1 - [x] Basic structure -> KILTprotocol/kilt-node#489 - [x] Merkleization of DID Documents -> KILTprotocol/kilt-node#492 - [x] `RuntimeCall` verification logic -> KILTprotocol/kilt-node#502 - [x] DID signature verification -> KILTprotocol/kilt-node#516 - [x] Add support for linked accounts and web3name -> KILTprotocol/kilt-node#525 - [x] Configurable origin for `commit_identity` -> KILTprotocol/kilt-node#526 - [x] Proper fee management -> KILTprotocol/kilt-node#528 - [x] Update to Polkadot 0.9.43 -> KILTprotocol/kilt-node@c18a6ce - [x] Replace XCM with state proofs -> KILTprotocol/kilt-node#543 - [x] Add support for relaychain consumer -> KILTprotocol/kilt-node#553 (part of KILTprotocol/kilt-node#543) - [x] Proper error handling -> KILTprotocol/kilt-node#572 - [x] Add support for versioning -> KILTprotocol/kilt-node#573 - [x] Take deposits for identity commitments -> KILTprotocol/kilt-node#574 - [x] Expose common definitions usable by consumers -> KILTprotocol/kilt-node#577 - [x] Change ensure_signed! to configurable origin also for the `dispatch_as` function -> KILTprotocol/kilt-node#577 - [x] Proper benchmarking and weights -> KILTprotocol/kilt-node#585 - [x] Comments and docs -> KILTprotocol/kilt-node#584 - [x] Revert Dockerfile changes in KILTprotocol/kilt-node#587 - [x] [OPTIONAL] Add support for Zombienet -> KILTprotocol/kilt-node#587 - [x] [OPTIONAL] Add chain spec loading from file for template runtimes -> KILTprotocol/kilt-node#587 - [x] Big, final review -> KILTprotocol/kilt-node#494 (review) - [x] Improvements n.1 PR -> KILTprotocol/kilt-node#591 - [x] Improvements n.2 PR -> KILTprotocol/kilt-node#592 - [x] Add to Peregrine runtime -> KILTprotocol/kilt-node#594 - [ ] Deploy on Peregrine - [ ] Unit tests - [ ] Add to Spiritnet runtime - [ ] Deploy on Spiritnet - [ ] [OPTIONAL] Move DIP-related stuff into its own repo --------- Co-authored-by: Adel Golghalyani <[email protected]> Co-authored-by: Chris Chinchilla <[email protected]> Co-authored-by: Albrecht <[email protected]>

Feature branch for everything DIP. It will collect other PRs until we are happy with the features, and will add the DIP to some of our runtimes and merge this into `develop`. ## WIP Checklist for the open tasks for v1 - [x] Basic structure -> #489 - [x] Merkleization of DID Documents -> #492 - [x] `RuntimeCall` verification logic -> #502 - [x] DID signature verification -> #516 - [x] Add support for linked accounts and web3name -> #525 - [x] Configurable origin for `commit_identity` -> #526 - [x] Proper fee management -> #528 - [x] Update to Polkadot 0.9.43 -> c18a6ce - [x] Replace XCM with state proofs -> #543 - [x] Add support for relaychain consumer -> #553 (part of #543) - [x] Proper error handling -> #572 - [x] Add support for versioning -> #573 - [x] Take deposits for identity commitments -> #574 - [x] Expose common definitions usable by consumers -> #577 - [x] Change ensure_signed! to configurable origin also for the `dispatch_as` function -> #577 - [x] Proper benchmarking and weights -> #585 - [x] Comments and docs -> #584 - [x] Revert Dockerfile changes in #587 - [x] [OPTIONAL] Add support for Zombienet -> #587 - [x] [OPTIONAL] Add chain spec loading from file for template runtimes -> #587 - [x] Big, final review -> #494 (review) - [x] Improvements n.1 PR -> #591 - [x] Improvements n.2 PR -> #592 - [x] Add to Peregrine runtime -> #594 - [ ] Deploy on Peregrine - [ ] Unit tests - [ ] Add to Spiritnet runtime - [ ] Deploy on Spiritnet - [ ] [OPTIONAL] Move DIP-related stuff into its own repo --------- Co-authored-by: Adel Golghalyani <[email protected]> Co-authored-by: Chris Chinchilla <[email protected]> Co-authored-by: Albrecht <[email protected]>

ntn-x2 added 5 commits November 1, 2023 12:05

Update pallet-dip-provider to allow generic hooks

03d40fd

WIP implementation for commitment storage deposit manager

ba54dab

Minor tweaks

80c909e

New pallet-deposit-storage compiling

bde05bf

Provider runtime compiling

8d9a2bc

ntn-x2 self-assigned this Nov 2, 2023

ntn-x2 changed the base branch from develop to aa/dip November 2, 2023 12:26

ntn-x2 commented Nov 2, 2023

View reviewed changes

dip-template/runtimes/dip-provider/src/dip.rs Show resolved Hide resolved

ntn-x2 commented Nov 2, 2023

View reviewed changes

dip-template/runtimes/dip-provider/src/dip.rs Outdated Show resolved Hide resolved

ntn-x2 added 12 commits November 2, 2023 13:28

Remove temp file

8deb833

Cleanup

52af061

Add reclaim_deposit extrinsic

e93d5c5

Minor chores

f80e2d4

Fix compilation issues

8041942

Add extrinsic to add deposit

8c749b1

Compiling

ee31f76

Cleanups

841a138

Half-way through deposit reclaiming hooks

b305769

Compiling

16f4a6f

Cleanup

68d6053

Make limits configurable by the runtime

66cbcbc

ntn-x2 marked this pull request as ready for review November 3, 2023 17:00

ntn-x2 requested review from trusch, weichweich and Ad96el November 3, 2023 17:00

ntn-x2 added 2 commits November 6, 2023 09:32

Replace Namespace with configurable type

634c1c1

Fix provider runtime template

169c1ee

Ad96el reviewed Nov 6, 2023

View reviewed changes

ntn-x2 added 2 commits November 6, 2023 12:07

Simplify runtime API implementation

24cb5ac

Open PR comments

f73778d

Ad96el approved these changes Nov 6, 2023

View reviewed changes

Ad96el reviewed Nov 7, 2023

View reviewed changes

pallets/pallet-dip-provider/src/lib.rs Outdated Show resolved Hide resolved

Remove unused constant

1841f0f

Ad96el reviewed Nov 7, 2023

View reviewed changes

trusch reviewed Nov 8, 2023

View reviewed changes

trusch approved these changes Nov 8, 2023

View reviewed changes

ntn-x2 added the 🗺️ DIP label Nov 8, 2023

This was referenced Nov 8, 2023

chore: improve DX #577

Merged

feat: DIP v1 features #494

Merged

ntn-x2 merged commit 6d2481e into aa/dip Nov 9, 2023
3 checks passed

ntn-x2 deleted the aa/dip-deposits branch November 9, 2023 15:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add deposits for DIP provider pallet #574

feat: add deposits for DIP provider pallet #574

ntn-x2 commented Nov 2, 2023 •

edited

Loading

Ad96el left a comment

Ad96el left a comment

Ad96el Nov 7, 2023

ntn-x2 Nov 7, 2023

ntn-x2 Nov 9, 2023

trusch Nov 8, 2023

ntn-x2 Nov 8, 2023

trusch Nov 9, 2023

ntn-x2 Nov 9, 2023

trusch left a comment

feat: add deposits for DIP provider pallet #574

feat: add deposits for DIP provider pallet #574

Conversation

ntn-x2 commented Nov 2, 2023 • edited Loading

Ad96el left a comment

Choose a reason for hiding this comment

Ad96el left a comment

Choose a reason for hiding this comment

Ad96el Nov 7, 2023

Choose a reason for hiding this comment

ntn-x2 Nov 7, 2023

Choose a reason for hiding this comment

ntn-x2 Nov 9, 2023

Choose a reason for hiding this comment

trusch Nov 8, 2023

Choose a reason for hiding this comment

ntn-x2 Nov 8, 2023

Choose a reason for hiding this comment

trusch Nov 9, 2023

Choose a reason for hiding this comment

ntn-x2 Nov 9, 2023

Choose a reason for hiding this comment

trusch left a comment

Choose a reason for hiding this comment

ntn-x2 commented Nov 2, 2023 •

edited

Loading