Extend multisig contract with weights #416
Conversation
husio
force-pushed
the
multisig_support_voting_weight_issue_285
branch
2 times, most recently
from
57a2a98 to
5730468
Compare
|
Is this ready for a review? |
Almost. I want to write a few more tests and double check that everything is implemented as expected. I will let you know when the code is ready for review. |
`x/multisig` extension was updated and now each signature is having a weight that defines what is the power of that signature. - each signature power/weight must be greater than 0, - activation threshold must be less or equal to the sum of all signature powers, - admin threshold can be greater than the sum of all signature powers. This can be used to create a contract that can be only activated but never changed, - multisig contract must have at least one participant/signature declared, - maximum number of participants is 100 in order to prevent CPU burning, resolve #285
husio
force-pushed
the
multisig_support_voting_weight_issue_285
branch
from
7095b77 to
275fbba
Compare
Codecov Report
@@ Coverage Diff @@
## master #416 +/- ##
========================================
+ Coverage 68.6% 68.6% +<.1%
========================================
Files 141 141
Lines 6936 6971 +35
========================================
+ Hits 4761 4788 +27
- Misses 1670 1677 +7
- Partials 505 506 +1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Nice job here.
I will approve it now, as I am not available to review again for a few hours, but please make a number of the changes I wrote below (or respond with a comment why it is a bad idea), and merge once you feel you addressed them all.
Also, note that this implements the POST approach (as before), not PATCH. So, maybe we will need a new PR to add that. (Just add a comment when closing the issue).
| authenticated := x.HasNAddresses(ctx, h.auth, sigs, int(contract.AdminThreshold)) | ||
| if !authenticated { | ||
| return nil, errors.Wrapf(errors.ErrUnauthorized, "contract=%X", updateContractMsg.Id) | ||
| if err := msg.Validate(); err != nil { |
There was a problem hiding this comment.
req: please move this up before L146 so that you validate the input data before you actually use them
There was a problem hiding this comment.
I moved it after checking the weights because I want to get ErrUnauthorized before any validation is executed. Otherwise, you can create a transaction without signing that will be validated.
Not sure if that is a bit deal but it feels more appropriate to check permissions before running any functionality.
What do you think?
There was a problem hiding this comment.
With other handlers like cash or escrow we do input validation before auth checks. I have not checked the others but it would be nice to be consistent
There was a problem hiding this comment.
I agree. We should validate the message first.
But not modify db state until we have auth
x/multisig/decorator_test.go
Outdated
| auth: auth, | ||
| bucket: NewContractBucket(), | ||
| } | ||
| res, err := handler.Deliver(ctx, db, &weavetest.Tx{Msg: &msg}) |
There was a problem hiding this comment.
pp: would be ok to persist the contract via the bucket directly. This test is about the decorator reading it only. Deliver should be checked in the handler_test
There was a problem hiding this comment.
Yes, great spot! Done in fd1a9bf
| return errors.Wrap(err, "admin threshold") | ||
| } | ||
|
|
||
| var total Weight |
There was a problem hiding this comment.
req: you need to check max power <256 as in the spec. Otherwise this can give you an overflow
x/multisigextension was updated and now each signature is having aweight that defines what is the power of that signature.
each signature power/weight must be greater than 0,
activation threshold must be less or equal to the sum of all signature
powers,
admin threshold can be greater than the sum of all signature powers.
This can be used to create a contract that can be only activated but
never changed,
multisig contract must have at least one participant/signature
declared,
maximum number of participants is 100 in order to prevent CPU burning,
🔔 Update the CHANGELOG and include the breaking change description.
resolve #285