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: DIP improvements #602

Merged
merged 49 commits into from
Feb 22, 2024
Merged

chore: DIP improvements #602

merged 49 commits into from
Feb 22, 2024

Conversation

ntn-x2
Copy link
Member

@ntn-x2 ntn-x2 commented Jan 15, 2024

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

It also contains a breaking change for cross-chain DID signature verifications. The signature now has a valid_until field, as discussed in #494 (comment).

Besides that, other changes include:

  • A new set of types representing different stages of a cross-chain DIP proof, during the verification process. Everything starts with either a RelayDipDidProof or a ParachainDipDidProof and ends, if the whole verification flow succeeds, with a DipVerifiedInfo.
  • A generic verify_storage_value_proof that is used to verify a single storage element with a storage proof.
  • The KiltVersionedParachainVerifier now also depends on the relaychain runtime, which removes the need for some traits that provided just type definitions, such as RelayChainStorageInfo and RelayChainStateInfo.
  • Errors conversions into u8 now start from 1 instead of 0, for disambiguating between an error returning u8::MAX and another case returning u8::MAX + 0.
  • Refactoring of the different modules.

No unit tests or benchmarks yet, but this code should make it easier to do all of those.

@ntn-x2 ntn-x2 marked this pull request as ready for review February 9, 2024 13:28
@ntn-x2
Copy link
Member Author

ntn-x2 commented Feb 9, 2024

@weichweich @Ad96el PR is ready for review! As I mentioned above, I will open a few other ones for unit tests, benchmarks, and improved logging, at least.

dip-template/runtimes/dip-consumer/src/dip.rs Show resolved Hide resolved
pallets/pallet-deposit-storage/src/deposit.rs Show resolved Hide resolved
LinkedDidInfoProviderError::DidNotFound => 0,
LinkedDidInfoProviderError::DidDeleted => 1,
LinkedDidInfoProviderError::TooManyLinkedAccounts => 2,
LinkedDidInfoProviderError::DidNotFound => 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. A comment that and why 0 isn't allows would be good.

runtimes/common/src/dip/merkle.rs Show resolved Hide resolved
runtimes/peregrine/src/dip/deposit.rs Show resolved Hide resolved
crates/kilt-dip-primitives/src/traits.rs Outdated Show resolved Hide resolved
@@ -0,0 +1,69 @@
// KILT Blockchain – https://botlabs.org
// Copyright (C) 2019-2023 BOTLabs GmbH
Copy link
Contributor

Choose a reason for hiding this comment

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

if this code is copied from somewhere, do we need to include a copyright notice from the source?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you think this is good now?

@@ -0,0 +1,1299 @@
// KILT Blockchain – https://botlabs.org
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be worth splitting this file up.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I wanted to do it in the testing PR, since it makes testing easier. I will probably leave this comment open and reference it in the ongoing testing PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will split this up as part of the unit test PR for that component, which is landing soon™!

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.

Mostly Coding style questions. 🙌
Would go over it again to understand more semantic aspects

pallets/pallet-deposit-storage/src/deposit.rs Show resolved Hide resolved
crates/kilt-dip-primitives/src/merkle/v0.rs Show resolved Hide resolved
crates/kilt-dip-primitives/src/utils.rs Show resolved Hide resolved
runtimes/common/src/dip/merkle.rs Show resolved Hide resolved
@ntn-x2 ntn-x2 enabled auto-merge (squash) February 22, 2024 07:07
@ntn-x2 ntn-x2 merged commit 62a1373 into develop Feb 22, 2024
2 checks passed
@ntn-x2 ntn-x2 deleted the aa/dip-improvements branch February 22, 2024 08:07
@ntn-x2 ntn-x2 mentioned this pull request Mar 1, 2024
5 tasks
ntn-x2 added a commit that referenced this pull request Mar 28, 2024
Partially fixes KILTprotocol/ticket#2562.

Adds unit tests for the verifier components (components tested are shown
in the checklist below).
It also splits up the proof verification logic into multiple files,
split by when those checks are performed. Check the
`crates/kilt-dip-primitives/src/merkle/v0/` folder for more details. It
addresses an open comment in the DIP refactoring PR:
#602 (comment).

## Elements to add tests for

- [x] `pallet-dip-consumer`
- [x] `pallet-relay-store`
- [x] `ProofVerifier`
- [x] Merge #612
- [x] Merge #613
Ad96el pushed a commit that referenced this pull request Apr 2, 2024
Fixes KILTprotocol/ticket#3054.

It also contains a **breaking change** for cross-chain DID signature
verifications. The signature now has a `valid_until` field, as discussed
in
#494 (comment).

Besides that, other changes include:

* A new set of types representing different stages of a cross-chain DIP
proof, during the verification process. Everything starts with either a
`RelayDipDidProof` or a `ParachainDipDidProof` and ends, if the whole
verification flow succeeds, with a `DipVerifiedInfo`.
* A generic `verify_storage_value_proof` that is used to verify a single
storage element with a storage proof.
* The `KiltVersionedParachainVerifier` now also depends on the
relaychain runtime, which removes the need for some traits that provided
just type definitions, such as `RelayChainStorageInfo` and
`RelayChainStateInfo`.
* Errors conversions into `u8` now start from `1` instead of `0`, for
disambiguating between an error returning `u8::MAX` and another case
returning `u8::MAX + 0`.
* Refactoring of the different modules.

**No unit tests or benchmarks yet, but this code should make it easier
to do all of those**.
Ad96el pushed a commit that referenced this pull request Apr 2, 2024
Partially fixes KILTprotocol/ticket#2562.

Adds unit tests for the verifier components (components tested are shown
in the checklist below).
It also splits up the proof verification logic into multiple files,
split by when those checks are performed. Check the
`crates/kilt-dip-primitives/src/merkle/v0/` folder for more details. It
addresses an open comment in the DIP refactoring PR:
#602 (comment).

## Elements to add tests for

- [x] `pallet-dip-consumer`
- [x] `pallet-relay-store`
- [x] `ProofVerifier`
- [x] Merge #612
- [x] Merge #613
ntn-x2 added a commit that referenced this pull request Apr 4, 2024
Fixes KILTprotocol/ticket#3054.

It also contains a **breaking change** for cross-chain DID signature
verifications. The signature now has a `valid_until` field, as discussed
in
#494 (comment).

Besides that, other changes include:

* A new set of types representing different stages of a cross-chain DIP
proof, during the verification process. Everything starts with either a
`RelayDipDidProof` or a `ParachainDipDidProof` and ends, if the whole
verification flow succeeds, with a `DipVerifiedInfo`.
* A generic `verify_storage_value_proof` that is used to verify a single
storage element with a storage proof.
* The `KiltVersionedParachainVerifier` now also depends on the
relaychain runtime, which removes the need for some traits that provided
just type definitions, such as `RelayChainStorageInfo` and
`RelayChainStateInfo`.
* Errors conversions into `u8` now start from `1` instead of `0`, for
disambiguating between an error returning `u8::MAX` and another case
returning `u8::MAX + 0`.
* Refactoring of the different modules.

**No unit tests or benchmarks yet, but this code should make it easier
to do all of those**.
ntn-x2 added a commit that referenced this pull request Apr 4, 2024
Partially fixes KILTprotocol/ticket#2562.

Adds unit tests for the verifier components (components tested are shown
in the checklist below).
It also splits up the proof verification logic into multiple files,
split by when those checks are performed. Check the
`crates/kilt-dip-primitives/src/merkle/v0/` folder for more details. It
addresses an open comment in the DIP refactoring PR:
#602 (comment).

## Elements to add tests for

- [x] `pallet-dip-consumer`
- [x] `pallet-relay-store`
- [x] `ProofVerifier`
- [x] Merge #612
- [x] Merge #613
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