feat: dynamic deposit for did creation #507
Conversation
pallets/did/src/lib.rs
Outdated
| let did_entry = Did::<T>::get(key); | ||
| match did_entry { | ||
| Some(entry) => entry.calculate_deposit(key), | ||
| _ => <T as Config>::BaseDeposit::get(), | ||
| } |
There was a problem hiding this comment.
A comment would be good here. Why take the base deposit when there is nothing in storage. This seems counter intuitive.
Would obviously be better if the interface would provide you with the item and not the key... I'm not 100% sure but there was some reason for adding the key and not the value here. 🤔
pallets/did/src/did_details.rs
Outdated
| #[cfg(not(feature = "runtime-benchmarks"))] | ||
| kilt_support::reserve_deposit::<AccountIdOf<T>, CurrencyOf<T>>(_who, _deposit_to_reserve)?; | ||
| Ok(()) |
There was a problem hiding this comment.
We cannot simply omit this part in the runtime benchmarks? We would not benchmark the weight that is added by this call, which is not what we want.
pallets/did/src/did_details.rs
Outdated
| let count_endpoints: BalanceOf<T> = DidEndpointsCount::<T>::get(did_subject).into(); | ||
| deposit = deposit.saturating_add(count_endpoints.saturating_mul(T::ServiceEndpointDeposit::get())); | ||
|
|
||
| let key_agreement_counts: BalanceOf<T> = self.key_agreement_keys.len().saturated_into(); | ||
| deposit = deposit.saturating_add(key_agreement_counts.saturating_mul(T::KeyDeposit::get())); |
There was a problem hiding this comment.
For anyone with an OCD it's count_endpoints and key_agreement_counts. Once it's in front the other time in the back. 😱 Maybe rename them to endpoint_count and key_agreement_count
pallets/did/src/did_details.rs
Outdated
| @@ -642,6 +639,30 @@ pub struct DidCreationDetails<T: Config> { | |||
| pub new_service_details: Vec<DidEndpoint<T>>, | |||
| } | |||
|
|
|||
| impl<T: Config> DidCreationDetails<T> { | |||
| pub fn calculate_deposit(&self) -> BalanceOf<T> { | |||
There was a problem hiding this comment.
Do we still need this if we have the same function for DidDetails?
There was a problem hiding this comment.
I need the function to evaluate the deposit before doing any heavy work. I think it is a design decision now. Likewise, I would leave it there, but if you think it is ugly and the computation power is to less to care about, I will remove it
There was a problem hiding this comment.
Is the heavy work you are referring to the one that checks the validity of the input? If that's the case, it does not matter much, since we take fees for that anyway, based on the size of the input, and we never return any surplus in case we finish early (e.g., if the user has not enough funds to pay for the deposit). Hence I would say it makes sense to avoid duplication and remove the code to calculate the deposit from the creation details, and simply call it on the created DidDetails inside the creation operation.
Alternatively, you could also simply create the details from the creation, calculate and check for the deposit, then run all the checks.
But as I said, the extrinsic is benchmarked depending on the input, so it does not really matter if we return early from the function or not.
runtimes/common/src/constants.rs
Outdated
| @@ -324,7 +324,18 @@ pub mod did { | |||
| /// The size is checked in the runtime by a test. | |||
| pub const MAX_DID_BYTE_LENGTH: u32 = 9918; | |||
|
|
|||
| pub const DID_DEPOSIT: Balance = deposit(2 + MAX_NUMBER_OF_SERVICES_PER_DID, MAX_DID_BYTE_LENGTH); | |||
| /// The length of all used Keys. | |||
| pub const KEY_DID_BYTE_LENGTH: u32 = 32; | |||
There was a problem hiding this comment.
Is is rather the max length of a key, right? In future if we use a key that is 33 byte long, we will probably bump this to 33, right?
There was a problem hiding this comment.
Yes exactly 👯. I updated the comment.
There was a problem hiding this comment.
Could you maybe rename the const as well? 😁
fixes 2636
Since the service endpoints would each support 2 URLs, the maximum DID size would also be bigger than before. To accommodate, the base deposit of around 2 KILT would remain, but the deposit could increase depending on heavy usage (calculated automatically if you rotate/add keys or add service endpoints). For example, having 25 service endpoints would increase the deposit needed, up to a maximum of 4 KILT. If you later delete the service endpoints and the DID gets smaller again the extra deposit would be automatically refunded.
The runtimeAPI call to get an approximation for the deposit will be developed in a different PR.