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

chore: improve DX #577

Merged
merged 47 commits into from
Nov 15, 2023
Merged

chore: improve DX #577

merged 47 commits into from
Nov 15, 2023

Conversation

ntn-x2
Copy link
Member

@ntn-x2 ntn-x2 commented Nov 8, 2023

Fixes https://github.com/KILTprotocol/ticket/issues/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

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)

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

	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

	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.

@ntn-x2 ntn-x2 self-assigned this Nov 8, 2023
@ntn-x2 ntn-x2 mentioned this pull request Nov 9, 2023
30 tasks
Base automatically changed from aa/dip-deposits to aa/dip November 9, 2023 15:02
Copy link
Member

@Ad96el Ad96el left a comment

Choose a reason for hiding this comment

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

LGTM. I'm considering adding a fourth runtime, where the required configuration traits for pallets are implemented. This would make integration easier for chains that want to use KILT as the provider. What do you think? I can provide a more detailed review in a week when I have a deeper understanding of the DIP implementation. If necessary, a second review can follow. :)

pallets/pallet-dip-provider/src/traits.rs Outdated Show resolved Hide resolved
crates/kilt-dip-support/src/utils.rs Outdated Show resolved Hide resolved
crates/kilt-dip-support/src/export/child.rs Show resolved Hide resolved
pallets/pallet-dip-consumer/src/lib.rs Outdated Show resolved Hide resolved
pallets/pallet-dip-provider/src/traits.rs Show resolved Hide resolved
@ntn-x2
Copy link
Member Author

ntn-x2 commented Nov 13, 2023

LGTM. I'm considering adding a fourth runtime, where the required configuration traits for pallets are implemented. This would make integration easier for chains that want to use KILT as the provider.

If you mean so that a consumer can call the Kilt-flavored struct to implement the ProofVerifier trait, then yes, that makes sense, I mentioned in the third point of the bullet list above. That would probably be exported inside the kilt-dip-support crate, in its own module and re-exported from the root. If you mean something else, then I don't think I understood your point.

@ntn-x2 ntn-x2 requested a review from Ad96el November 13, 2023 16:31
Copy link
Member

@Ad96el Ad96el left a comment

Choose a reason for hiding this comment

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

I would likely review it once more, if I have the time, but I believe it's ready to be merged

Copy link
Contributor

@trusch trusch left a comment

Choose a reason for hiding this comment

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

This looks nice, especially the Kilt specific types will make it super easy for consumers to integrate with us without loosing the generic types that would work with any provider!

@ntn-x2 ntn-x2 merged commit 3ce3da6 into aa/dip Nov 15, 2023
2 checks passed
@ntn-x2 ntn-x2 deleted the aa/dip-better-re-export branch November 15, 2023 09:00
ntn-x2 added a commit that referenced this pull request Dec 14, 2023
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]>
webguru9178 pushed a commit to webguru9178/kilt-node that referenced this pull request Jan 8, 2024
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]>
Ad96el added a commit that referenced this pull request Feb 7, 2024
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]>
Ad96el added a commit that referenced this pull request Apr 2, 2024
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants