Skip to content
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 executeConfirm method to perform multi-sig in 1 transaction #68

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bencxr
Copy link
Contributor

@bencxr bencxr commented Jul 11, 2016

Support for executing and confirmation an outgoing send from the wallet in a single transaction.
msg.sender is used for one of the signatures and data with ecrecover is used for another "signature".

How to generate the signature with eth.sign:
uint expireTime = 1863771845; // 10 years in the future
uint sequenceId = 1; // or the next sequence Id obtained using getNextSequenceId();
bytes32 sha3 = sha3(to, value, data, expireTime, sequenceId); // see tests for examples how to build this
bytes signature = eth.sign(owner1, sha3); // sign the sha3 using owner1

Example usage in tests:
https://github.com/BitGo/eth-multisig-v2/blob/master/test/wallet.js#L1075

@veox
Copy link

veox commented Aug 2, 2016

Sorry, too tired to continue line-by-line right now, but I see there's the same tendency (as in #65) to omit constant and internal specifiers; and using var in cycles (most places I saw comparing to c_maxSequenceIdWindowSize, which seems to be a uint constant of 10, which could slide).

@bencxr
Copy link
Contributor Author

bencxr commented Aug 8, 2016

Thanks, I just addressed those concerns. However the bulk of this code does more heavy lifting so would appreciate a review of the new methods.

@chriseth
Copy link
Contributor

I would prefer this change to be more modular. Would it be a possibility to create a "confirmation" module that handles the actual confirmations - which can be done using signatures only or direct calls?

@vbuterin
Copy link
Contributor

vbuterin commented Aug 24, 2016

Great work; this is something that is very much needed.

+1 to chriseth's comment that this could be more modular. Also, a nicer way of doing all of this might be to have the confirm function take an array of signatures, which could be empty (ie. the only confirm is the confirm from the tx sender), have 1 sig, or have multiple sigs; this way we need fewer functions.

@bencxr
Copy link
Contributor Author

bencxr commented Aug 30, 2016

@vbuterin currently solidity does not support an array of bytes (signatures). It's actually rather easy to cal/use this method now, and I would rather not add additional complexity on the client if possible. We could add on a function taking when solidity does support array of bytes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants