Conversation
ntn-x2
left a comment
ntn-x2
left a comment
There was a problem hiding this comment.
Mostly looks good. I have some minor points about the migration stuff and few typos in the staking pallet. The biggest point is about the use of .uwnrap() inside the did::create pallet, which is no-good for the runtime logic. It is ok for it to fail instead.
ntn-x2
left a comment
ntn-x2
left a comment
There was a problem hiding this comment.
I think a lot has been improved since when the PR has been opened. I am happy with all the changes made. As I said, the only thing that might need to be done in a separate PR is a multi-block migration strategy, if needed. Also, we need to check that the logic for account reaping/dust collection has remained the same. I think @weichweich will still have to review this, at some point. But I will approve it on my side.
| fn remove_attestation(attestation: AttestationDetailsOf<T>, claim_hash: ClaimHashOf<T>) { | ||
| kilt_support::free_deposit::<AccountIdOf<T>, CurrencyOf<T>>(&attestation.deposit); | ||
| fn remove_attestation(attestation: AttestationDetailsOf<T>, claim_hash: ClaimHashOf<T>) -> DispatchResult { | ||
| AttestationStorageDepositCollector::<T>::free_deposit(attestation.deposit)?; |
There was a problem hiding this comment.
What are possible errors when freeing the deposit and what would this mean for remove_attestation?
I fear that if the error occurs, that there is no possible option to remove the attestation without the governance?
There was a problem hiding this comment.
There are two possible cases where remove attestation can fail:
- The user does not have enough balance on their hold, and the decrease should be
Exact. If the precision is set toBestEffort, the actual amount is returned in anOkresponse. - Even after increasing the free balance, the user is still under the ED.
As I see it, both cases can be ignored. In the first case, the actual deposit is tracked in the storage. Additionally, when an attestation is removed, we use BestEffort as the precision.
| Attestations::<T>::iter() | ||
| .map(|(key, attestations_detail)| -> Weight { |
There was a problem hiding this comment.
How many attestations do we have? I'm afraid that this is not an option for us? Especially since we not only iterate over all attestations but basically over all storage?
|
Looks good. But the migration has to be redone. I don't think that we can do that in a single block. I propose we do a lazy migration with a permission less call to update them. Like it was suggested in the PR. |
| @@ -562,7 +579,7 @@ dependencies = [ | |||
| [[package]] | |||
| name = "binary-merkle-tree" | |||
| version = "4.0.0-dev" | |||
| source = "git+https://github.com/paritytech/substrate?branch=polkadot-v0.9.41#3bd809d59c31c7205a0f36c6ddcba603e43051fc" | |||
| source = "git+https://github.com/paritytech/substrate?branch=polkadot-v0.9.42#3bb3882ce094ac211dea93fabfdcd4f902f5a7fd" | |||
There was a problem hiding this comment.
You don't seem to be up to date with polkadot-v0.9.42. Try running cargo update. This will update all dependencies. You could run cargo update -p sp-io which updates most of the substrate dependencies. Make sure to also check cumulus & polkadot repositories.
ntn-x2
left a comment
ntn-x2
left a comment
There was a problem hiding this comment.
Did not go over everything, but I like this approach more of migrating a specific account to the new version.
| assert!(attestation_post_migration.clone().unwrap().deposit.version.is_some()); | ||
| assert!(attestation_post_migration.unwrap().deposit.version.unwrap() == 1); | ||
|
|
||
| //Nothing should happen |
There was a problem hiding this comment.
Maybe you can keep the comment?
Ad96el
force-pushed
the
chore/polkadot-0.9.42
branch
from
dc34cbc to
b2fe45e
Compare
3 participants
fixes KILTProtocol/ticket#2693
Updates to Polkadot dependencies have been made to version 0.9.42. You can find all the changes here.
How to test
Since the Currency trait has been silently deprecated, the unit tests have been adjusted to support the newly introduced fungible traits. Pre- and post-validations have been implemented for the migrations.
Still to be updated
In all pallets where a negative imbalance is used, the Currency trait is still being used. The feecollector (the treasury pallet) has not been updated yet, and there is an open PR addressing this. If the PR is merged, we will be able to completely remove the Currency trait. Currently, the trait is still used in the staking, DID, and CType pallets.
Checklist:
array[3]useget(3), ...)