Port multisig example to low level data structures #1078
Conversation
athei
requested review from
ascjones,
cmichi,
HCastano and
Robbepop
as code owners
There's no way to get the length of a `Mapping` so it doesn't make sense to check this in tests anymore
|
Since we don't have the
A gain of 17K, not bad 😎 |
| pub struct Mapping<K, V> { | ||
| offset_key: Key, | ||
| _marker: PhantomData<fn() -> (K, V)>, | ||
| } | ||
|
|
||
| /// We implement this manually because the derived implementation adds trait bounds. |
There was a problem hiding this comment.
What additional trait bounds are added here?
I checked the cargo-expand-ed contracts using both versions and there was no difference between them.
There was a problem hiding this comment.
The Default derive requires Default for each type parameter. But for this Mapping we don't need these bounds. In substrate we have a DefaultNoBounds derive for this.
examples/multisig/lib.rs
Outdated
| if self.confirmations.get(&key).is_some() { | ||
| self.confirmations.remove(&key); | ||
| let mut count = self.confirmation_count.get(&trans_id).unwrap_or(0); | ||
| count += 1; |
There was a problem hiding this comment.
Hmm, seems it is a bug, it should be -1=)
There was a problem hiding this comment.
You are absolutely right. Thanks for catching this.
|
I tried to implement multisig by another way, I think it is more clear=) #1079 What do you think? |
I think the idea is interesting. But please let us merge this "port only" PR first once the waterfall is up and running. Your PR adds some further changes to the contract and also breaks the interface. This isn't bad but I would like to do this separately (it is based on this PR anyways). |
|
Removing lazy from |
Codecov Report
@@ Coverage Diff @@
## master #1078 +/- ##
===========================================
- Coverage 78.77% 62.69% -16.09%
===========================================
Files 248 248
Lines 9362 9360 -2
===========================================
- Hits 7375 5868 -1507
- Misses 1987 3492 +1505
Continue to review full report at Codecov.
|
|
These are the final numbers I get: Gonna merge this since it's quite an improvement over what we have. |
* Fix `Default` impl of `Mapping` * Replace HashMap by Mapping * Use initialize_contract * Initialize everything within `initialize_contract` * Make owners lazy * Remove commented out code There's no way to get the length of a `Mapping` so it doesn't make sense to check this in tests anymore * Fix - + bug * No need to make a primitive lazy * Clippy Co-authored-by: Hernando Castano <[email protected]>
This removes all usages of data structures in
collections. The new code shows some pain points of ink! and the pallet-contracts when removing those collections. I think this is exactly the reason why we should port this example with sub optimal code: We can come back, iterate and observe the improvements.These are some pain point of the contract which could be fixed but would change the interface so I did leave them as is:
Vec. The complexity of adding and removing owners scales linearly with the amount of owners. We might need to cap them.It also shows some shortcomings of the pallet-contracts map interface. I think we want the following functionality to paller-contracts:
take)