Improve signature collection UX #6174
Conversation
|
👋 Thanks for creating a PR! Before we can merge this PR, please make sure that all the following items have been For contributor use:
For admin use:
Thank you for your contribution to the Cosmos-SDK! 🚀 |
aaronc
changed the title
aaronc
changed the title
| @@ -54,33 +54,57 @@ package cosmos_sdk.v1; | |||
|
|
|||
| message Tx { | |||
| TxBody body = 1; | |||
| repeated bytes signatures = 2; | |||
| AuthInfo auth_info = 2; | |||
| repeated bytes signatures = 3; | |||
There was a problem hiding this comment.
How do you map signatures to the signers' pubkeys when AuthInfo.signer_infos is not sorted?
There was a problem hiding this comment.
SignerInfo should be in the same order as signatures. This just proposes that the order of signers does not need to match the SDK's existing requirement about signature ordering which you pointed out could be hard for a client to compute.
| PublicKey public_key = 1; | ||
| // address can be used for accounts that already have a public key in state | ||
| bytes address = 2; | ||
| } |
There was a problem hiding this comment.
Since this is not part of the TxBody anymore, this is only needed suring signing, right? So we can just use pubkey here (to reduce complexity) like we do right now in the StdSignature type.
There was a problem hiding this comment.
This is needed for both signing (by the primary signer) and tx broadcasting. Allowing the option to use address can reduce the tx size which could be significant if using a multisig pubkey.
| // message handlers that examine the full list of signers on the transaction | ||
| // context | ||
| repeated SignerInfo signer_infos = 1; | ||
| uint64 gas = 2; |
There was a problem hiding this comment.
Is this a gas_limit? A little doc comment what this field means would help, just a one-liner.
| // Each signer can specify a fee that they are paying separately. In the case | ||
| // that the chain supports refunds for unused gas, refunds will be distributed | ||
| // to the last signer first. | ||
| Fee fee = 4; |
There was a problem hiding this comment.
This moves the fee and fee payer out of the signed content, right? Interesting. I'm not sure about all the consequences this has.
How can the signers coordinate such that enough fee is payed? Or is the first signer still the primary fee payer?
There was a problem hiding this comment.
In this model there could be more than one fee payer and the first signer would be the primary. But maybe it's not needed to have multiple fee payers?
There was a problem hiding this comment.
I don't think multiple fee payers are needed.
In the typical multisig case that I am aware of, a few employees sign a transaction and the company pays (the group's shared account) pays the fee. I don't know if that is possible right now.
There was a problem hiding this comment.
I'll remove the multiple payers then and I think that will make this a lot easier to understand
| } | ||
| ``` | ||
| * **No Malleability**: given a `TxBody` and an `AuthInfo` there is one and only | ||
| one valid `Tx` that can be generated |
There was a problem hiding this comment.
This is impossible to get, as described in https://medium.com/@simonwarta/signature-determinism-for-blockchain-developers-dbd84865a93e. Most likely it is not needed as well.
| 2. Sign the encoded `SignDocAux` bytes | ||
| 3. Send their signature and `SignerInfo` to the tx composer(s) who will then | ||
| sign and broadcast the final transaction (with `SIGN_MODE_DIRECT` and fees and | ||
| gas added) once enough signatures have been collected |
There was a problem hiding this comment.
Why would the tx composer be a signer? It can be a moderator/secretary/notary/bot that is just responsible for composing a transaction, collection the signatures, putting everything together and broadcasting it.
There was a problem hiding this comment.
So maybe this can be the primary signer who is the fee payer. The composer can be anyone. And anyone who had collected enough auxiliary signatures could sign the message with a fee.
| * **No Malleability**: given a `TxBody` and an `AuthInfo` there is one and only | ||
| one valid `Tx` that can be generated | ||
| * **Predictable Gas**: if I am signing a transaction where I am paying a fee, | ||
| the final gas is fully dependent on what I am signing |
There was a problem hiding this comment.
Can you help me understand this point better: do we (right now and in the future) support an undefined number of signatures?
If not, the only purpose of this added complexity is to set different gas prices for different verification modes?
There was a problem hiding this comment.
As I understand it, with multisigs currently it could be unpredictable because the signatures are nested. Also, I am noting this in contrast to weave where my reading is that it's generally unpredictable.
|
Since some of you have mentioned this was hard to understand, here is my attempt at explaining this PR more simply is:
|
addresses the concerns raised in #6111 (comment)
Note that this does not address multisig public keys, public key encoding or reserved field ranges. I intend to address those separately.