Add refresh functionality to MultisigOperations #96
Conversation
|
|
||
| static validatePayload(payload: any) { | ||
| static _validatePayload(payload: any) { |
There was a problem hiding this comment.
What's the meaning of the underscore _ pre-pended method names ?
Does it still mean they're internal-only methods, or does it have some other meaning in this context ?
There was a problem hiding this comment.
Chris suggested making the internal/pseudo-private methods explicit; no other meaning. I think it's a good move, but it does conflict with the no-underscore-dangle rule.
There was a problem hiding this comment.
but it does conflict with the no-underscore-dangle rule.
That isn't / shouldn't be a problem. We can make our own rules if they make sense and as long as we're all on the same page. 👍
|
Thanks Raul. I think it should be safe, but we can also run it by Chris as he has had some more exposure to it 👍 |
| // If the nonce changed, the signers are no longer valid | ||
| this.signers = {}; | ||
| // eslint-disable-next-line no-console | ||
| console.warn('Nonce value changed; signers have been reset'); |
There was a problem hiding this comment.
Hmmm. I'm not sure about this. I think we should give developers a way to hook into this when this happens as they would like to report it back to their users. This kind of "silently" resets all the signers and will yield the wrong message for the users (signers are missing, even though they signed). If I understand this implementation correctly, I'm missing a way to notify the developer (and thus the user) that the nonce was reset in a better way.
Just thinking out loud here: how about a function that the dev defines which is called as soon as the signers are reset?
There was a problem hiding this comment.
Great idea. I've gone ahead and done that. I think it's fine to use just one function for now, rather than go full EventEmitter.
| const oldNonce = Number(this.nonce); | ||
| const newNonce = await this.sender.getNonce(); | ||
| if (oldNonce !== newNonce) { | ||
| this.nonce = newNonce; |
There was a problem hiding this comment.
🤔 Agreed, not helpful
| if (oldNonce !== newNonce) { | ||
| this.nonce = newNonce; | ||
| // If the nonce changed, the signers are no longer valid | ||
| this.signers = {}; |
There was a problem hiding this comment.
🤔 Agreed, we have the getters
* Adds a `refresh` method, so that the MultisigOperation can be partially-reset if the required signees or nonce value changes (i.e. we don't need to create a new one from scratch). It also provides an extra level of validation, so that we can check that the signees and nonce are current before sending the transaction. * Removes `nonce` from the payload; it doesn't need to be immutable * Adds a `requiredSignees` getter * Adds a `missingSignees` getter * Sets the nonce from within the operation, not from the Sender * Makes private functions clear (with underscores) * Makes the public methods chainable * Improves test coverage
JamesLefrere
force-pushed
the
feature/get-required-signees-on-multisig
branch
from
6516ad4
to
8649e22
Compare
Add pull docker image to npx instructions
Description
This PR aims to make the MultisigOperation a little more fool-proof by adding refresh functionality.
refreshmethod, so that the MultisigOperation can be partially-reset if the required signees or nonce value changes (i.e. we don't need to create a new one from scratch). It also provides an extra level of validation, so that we can check that the signees and nonce are current before sending a transaction which would otherwise fail.noncefrom the payload; it doesn't need to be immutable and is most likely to changeOther changes
requiredSigneesgettermissingSigneesgetter