-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add multi-sig support #300
Conversation
694413d
to
5413da0
Compare
5413da0
to
65ea4b4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good :)
Regarding UpdateCredentials
, I do not agree that access to ledger + desktop wallet is required for creating a valid credential file. You can do that with the tool client
in concordium-base/rust-bins/src/bin/
:)
54b1dcf
to
72eece2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I think it would be nice for submitting transactions to have better error handling (or perhaps checking before-hand) for the transaction having insufficient signatures or having already expired.
I also wonder if we can just completely remove the "sign-and-submit" behaviour. Part of the problem with it is that it uses a different format from "submit" and the other transaction commands, so it could cause issues for people.
It could also be nice to have an easy way to create a transaction without signing at all. (It is possible by specifying a file with contents {}
to the --keys
, so I don't think this is essential to add.)
Great input: I created an extra issue for checking for enough signatures since this behavior is already present and should be improved for regular transaction (not loaded from a partially signed transaction JSON file) as well. I added checks for the Yes, we should remove the I will add to the developer documentation your hack on creating a partially-signed transaction file without signing it with any keys. This can be useful if one tech-savvy person prepares the file but other people need to sign it. |
5bd2160
to
b423ec5
Compare
Purpose
Addresses #296
Related Concordium/concordium-base#529
The following transaction types were tested using multi-sig:
DeployModule
,InitContract
,Update
,Transfer
,TransferWithMemo
,TransferWithScheduleAndMemo
,TransferWithSchedule
,ConfigureDelegation
,ConfigureBaker
,RegisterData
,UpdateCredentialKeys
, andUpdateCredentials
.The following transaction types were NOT tested using multi-sig:
AddBaker
,RemoveBaker
,UpdateBakerStake
,UpdateBakerRestakeEarnings
, andUpdateBakerKeys
(concordium-client created these types in protocol versions belowTypes.P4
but not anymore).https://github.com/Concordium/concordium-client/blob/main/src/Concordium/Client/Runner.hs#L3698
UpdateCredentials
(access to ledger + desktop wallet is required for creating a valid credential file - postponed/blocked but planned to be tested later)Update: it was tested by now
TransferToEncrypted
,TransferToPublic
,EncryptedAmountTransfer
andEncryptedAmountTransferWithMemo
(shielded transactions are faced out and the feature will be removed, so the multi-sig feature will not be tested/implemented for it).
How to test this PR:
Use these two guides to create a multi-sig account.
https://gist.github.com/DOBEN/683fe1a7c82a0551546a7ec242d30cc0
https://gist.github.com/limemloh/8c0c55f67cf5a83ac7cc21cb646e65c1
There are two main steps to complete:
update-key
transaction on-chain to associate an additional key to your credential/account + set the key threshold to 2 (alternatively you can also add an additional credential with some keys)concordium-client
to locally have access to these two keys.Example transaction flow:
transaction.json
file:Changes
Rename subcommand
TransactionSubmit
toTransactionSignAndSubmit
(keep its functionality)Create two new subcommands:
TransactionSubmit
TransactionAddSignature
Add optional
--out
flag to all transaction-creating commands.