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

Issue867 Add CLI command to add signatures to a partially signed transaction #1032

Merged
merged 32 commits into from Mar 13, 2019

Conversation

@cifer-lee
Copy link
Member

commented Jun 7, 2018

Fix for #867

Note by @abitmore: the final implementation of this PR is online-signing but not offline-signing.

--- below is original message by @cifer-lee ---

Roughly implemented multi-sig approach.

Some explanation:

The new added multisig_trx_data struct was in consideration of user experience, thus when import the transaction we will know what keys are needed to sign it. But I'm not sure if this was a appropriate practice, if it was, I feel like to show which keys were already signed and which were not --- so make it more user friendly.

Would like to hear from more devs.

@pmconrad
Copy link
Contributor

left a comment

IMO this API is much too complicated. Specifically, it introduces a new type of state into the wallet API, which can lead to surprising behaviour.

I think the desired functionality can be implemented with a simple idempotent call - tx with no/few sigs in, tx with more/all sigs out.

@cifer-lee

This comment has been minimized.

Copy link
Member Author

commented Jun 8, 2018

@pmconrad thanks very much~ I have added some explanation on this topic. The new introduced type was intend to easily use..
Maybe you are right, I should implemented it simpler..

@ryanRfox

This comment has been minimized.

Copy link
Member

commented Jun 8, 2018

@cifer-lee Based on your comments above, it seems you are now refactoring Issue #867. Therefore, I will Label this PR as 2 Developing. Once you complete development on the Issue #867 , notify me and I will mark it 2 Approved (by you the developer, because it passes your testes). Next, I will mark this PR as 2 Final Review which will prompt @pmconrad to review your updates. Ultimately, he will mark merge the PR and Close the Issue.

This whole flow/organization is still a work in progress, so appreciate your comments for improvement.

@ryanRfox ryanRfox moved this from In Testing to In Development in Community Claims Jun 8, 2018

@jmjatlanta
Copy link
Contributor

left a comment

I have not tested this code (will do so soon), but did find a few areas where you may want to fix the log messages...

Show resolved Hide resolved libraries/wallet/wallet.cpp Outdated
Show resolved Hide resolved libraries/wallet/wallet.cpp Outdated

@ryanRfox ryanRfox added this to To Do in Feature Release (201808) via automation Jun 13, 2018

@cifer-lee

This comment has been minimized.

Copy link
Member Author

commented Jun 13, 2018

Thanks Peter and John~ @pmconrad I have made a lot of simplification according to your suggestion, may you check it again~

So far, self test ok:

  1. got a signed_transaction output using any existing api
  2. stripped spaces of the signed transaction and pasted to call with multisig_sign_transaction
  3. got a multi-signed transaction output
@jmjatlanta

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2018

As for the method name, add_transaction_signature

Pro: kind of indicates that others can add their signature as well.
Con: functionality is very similar to sign_transaction but the name does not make that obvious.

@cifer-lee

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2018

I think sign_transaction2 is ok in here, the difference with sign_transaction is just whether or not keep the origin signatures.

@cogutvalera

This comment has been minimized.

Copy link
Member

commented Oct 10, 2018

I do not think sign_transaction2 is very meaningful. It doesn't make sense for me.
add_transaction_signature as @jmjatlanta suggests makes more sense for me.
IMHO add_partially_signed_transaction_signature is more clear.

@ryanRfox ryanRfox removed this from In progress in Feature Release (201902) Feb 1, 2019

@ryanRfox ryanRfox added this to In development in Feature Release (3.1.0) via automation Feb 1, 2019

@ryanRfox ryanRfox assigned cifer-lee and unassigned ryanRfox Feb 12, 2019

@ryanRfox

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

@cifer-lee please review the Reviewer comments above and update the PR accordingly. I added this to the 201904 Feature Release.

@cifer-lee

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2019

@cifer-lee please review the Reviewer comments above and update the PR accordingly. I added this to the 201904 Feature Release.

@ryanRfox Okay, sorry for the delay of work, I will pick it up this afternoon

@cifer-lee

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2019

As for the method name, add_transaction_signature

Pro: kind of indicates that others can add their signature as well.
Con: functionality is very similar to sign_transaction but the name does not make that obvious.

I think we need a name to convey the meaning of "sign this transaction to add new signatures while keeping the existing signatures unchanged". So I think a name like sign_transaction_keep_existing_signatures is more meaningful.. But it's really long so actually I still think sign_transaction2 is a appropriate name, using number suffix in function name is not a bad practice, even well-known projects adopt this naming convention too, e.g. open2 dup2 in linux source code.

IMHO, sign_transaction2 is a nice name in two aspects:

  1. It's short, thus easy to read
  2. It clearly indicates that it's another version of sign_transaction, just with minor difference

We just need clearly describe the difference in document.

@jmjatlanta @abitmore @cogutvalera please reconsider it?


Update:

I just come up with a few new names: sign_transaction_incrementally / sign_transaction_by_append. I'm not sure if users can catch the point. How do you think about this name?

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

IMO add_transaction_signature is sufficiently clear in that it only adds something, which implies that it does not remove existing signatures.
Much better than sign_transaction2, which gives no indication in what way it is different than sign_transaction. (Note that in your dup2 example, 2 is the number of arguments which is different from dup which has only 1 argument.)

lixiaodongcifer and others added some commits Feb 16, 2019

@abitmore

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

Sorry, #1626 introduced a conflict, please fix it. Thanks.

other developers have different opinions

@cifer-lee

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2019

Oh, have resolved the conflicts

@pmconrad pmconrad moved this from In development to In testing in Feature Release (3.1.0) Mar 5, 2019

@abitmore abitmore merged commit de31b56 into bitshares:develop Mar 13, 2019

2 checks passed

ci/dockercloud Your tests passed in Docker Cloud
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Feature Release (3.1.0) automation moved this from In testing to Done Mar 13, 2019

@pmconrad pmconrad removed this from In Testing in Community Claims Apr 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.