-
Notifications
You must be signed in to change notification settings - Fork 47
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: add unit tests for DIP components #609
Conversation
e6f89b6
to
e5dff16
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice changes and good test coverage 🙌 .
We should also include some try_runtime
tests for the provider pallet.
Some concerns about the logging. I would change them debug logs.
I guess the file structure can be simplified.
pallets/pallet-deposit-storage/src/deposit/tests/on_commitment_removed.rs
Show resolved
Hide resolved
pallets/pallet-deposit-storage/src/deposit/tests/on_commitment_removed.rs
Show resolved
Hide resolved
@Ad96el I agree, but as long as we don't have any migration logic I though to spare some time and skip it, for now. What benefit would it give us at this point? I'm all ears. |
I was thinking of having sanity tests. We could test that each commitment has an entry in the deposit pallet. For that, we need the try-runtime tests. If I remember correctly, this is not tested or? Please correct me if am wrong. |
This is a good test to add, I agree! I would still rather keep the pallets isolated from each other tho, as they have been designed. So what is the best strategy to do this with |
Yes, try-runtime only works on pallet level. So I guess the best way is, that the trait itself have a function which is feature guided via try-runtime. So we can still guarantee it is generic for other chains, but we can introduce them in our chain. |
Ok, thanks for the input. I will think if this is good or if it defeats any decoupling principles. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Partially fixes KILTprotocol/ticket#2562. ## Tests for the DIP provider components The DIP provider components are tested in this PR. Specifically: * The `pallet-deposit-storage` pallet main logic. * The `DepositHooks` hook for deleting an identity commitment when a deposit is released directly. It used to leave in the Peregrine runtime but it now lives in `runtime-common` and is generic over the runtime. This was required for testing the hook logic. * The `pallet-dip-provider` pallet main logic. * The `FixedDepositCollectorViaDepositsPallet` hook for reserving and releasing deposits when identity commitments are deleted. It uses a different mock than the pallet mock. * The `generate_proof` and `generate_commitment` functions, along with the `DidMerkleRootGenerator`. * The `LinkedDidInfoProvider`. The PR also contains minor refactoring that was necessary to do proper mocking of the different pieces of the runtime to test the components above. The main changes are: * Move from a multi-module to a single-module approach, where files do not contain more than a module. * Move the `DepositHooks` from being Peregrine-specific to being runtime-agnostic, in `runtime-common`. **This PR only adds tests for the DIP provider components. The DIP consumer will be tested in a separate PR.**
Partially fixes KILTprotocol/ticket#2562. ## Tests for the DIP provider components The DIP provider components are tested in this PR. Specifically: * The `pallet-deposit-storage` pallet main logic. * The `DepositHooks` hook for deleting an identity commitment when a deposit is released directly. It used to leave in the Peregrine runtime but it now lives in `runtime-common` and is generic over the runtime. This was required for testing the hook logic. * The `pallet-dip-provider` pallet main logic. * The `FixedDepositCollectorViaDepositsPallet` hook for reserving and releasing deposits when identity commitments are deleted. It uses a different mock than the pallet mock. * The `generate_proof` and `generate_commitment` functions, along with the `DidMerkleRootGenerator`. * The `LinkedDidInfoProvider`. The PR also contains minor refactoring that was necessary to do proper mocking of the different pieces of the runtime to test the components above. The main changes are: * Move from a multi-module to a single-module approach, where files do not contain more than a module. * Move the `DepositHooks` from being Peregrine-specific to being runtime-agnostic, in `runtime-common`. **This PR only adds tests for the DIP provider components. The DIP consumer will be tested in a separate PR.**
Partially fixes https://github.com/KILTprotocol/ticket/issues/2562.
Tests for the DIP provider components
The DIP provider components are tested in this PR. Specifically:
pallet-deposit-storage
pallet main logic.DepositHooks
hook for deleting an identity commitment when a deposit is released directly. It used to leave in the Peregrine runtime but it now lives inruntime-common
and is generic over the runtime. This was required for testing the hook logic.pallet-dip-provider
pallet main logic.FixedDepositCollectorViaDepositsPallet
hook for reserving and releasing deposits when identity commitments are deleted. It uses a different mock than the pallet mock.generate_proof
andgenerate_commitment
functions, along with theDidMerkleRootGenerator
.LinkedDidInfoProvider
.The PR also contains minor refactoring that was necessary to do proper mocking of the different pieces of the runtime to test the components above. The main changes are:
DepositHooks
from being Peregrine-specific to being runtime-agnostic, inruntime-common
.This PR only adds tests for the DIP provider components. The DIP consumer will be tested in a separate PR.