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 CL flag support to sidechain manipulation script #60

Merged
merged 1 commit into from
Nov 18, 2015

Conversation

droark
Copy link
Contributor

@droark droark commented Nov 9, 2015

  • Add support for command line arguments to the sidechain manipulation script. argparse requires Python 2.7 or 3.2+.
  • Add support for unlocking mainchain wallet when running send-to-sidechain.
  • Add various bits of help text, some of which may not be quite right.

@droark droark changed the title Add CL arg support to sidechain manipulation script Add CL flag support to sidechain manipulation script Nov 9, 2015
@droark
Copy link
Contributor Author

droark commented Nov 9, 2015

This is a more thorough (and tested) replacement for PR #44. I ended up replacing the sys.argv usage with argparse. This way, the code adds a more useful help system alongside efficient argument parsing. The one and only time that CL flags are used right now are to unlock Bitcoin wallets when using send-to-sidechain. I discovered that the code, when Armory is running, will use a locked Armory wallet. That's how I tested this.

Note that HEAD won't work due to libsecp256k1 API breakage. See issue #59 for details regarding where to rewind libsecp256k1 & contracthashtool in order to test this patch.

Thanks.

@droark
Copy link
Contributor Author

droark commented Nov 10, 2015

Paging @gwillen. Am I on the right path? Did I go too far with the changes?

@gwillen
Copy link
Contributor

gwillen commented Nov 10, 2015

No way, you didn't go too far at all, this is awesome. Let me test it out and then figure out what the process is for us to merge it. Thanks!

@droark
Copy link
Contributor Author

droark commented Nov 10, 2015

Thanks, and sorry about the extra commits that just popped up! I added a commit and tried to squash it. My squash mojo just isn't right today, unfortunately. I'll clean it up one way or another.

@gwillen
Copy link
Contributor

gwillen commented Nov 10, 2015

No problem. I think what you want is 'git rebase -i foo' -- starting with the branch whose tip you want to squash checked out -- where foo is the parent of the first commit you want to squash; then replace 'pick' with 'squash' on all lines but the first: http://stackoverflow.com/questions/5189560/squash-my-last-x-commits-together-using-git

And then 'git push -f [something appropriate]' to update the pullreq. (Which will nuke all inline comments on the previous commits unfortunately, but in this case there aren't any.)

@droark
Copy link
Contributor Author

droark commented Nov 10, 2015

Thanks! That did the trick. I had basically done that. I didn't force the initial push, though, so something went sideways. Oh well. All's well now.

@gwillen
Copy link
Contributor

gwillen commented Nov 16, 2015

Other than the comment above about the behavior in case of failure -- which I think should emulate the old behavior as much as possible if the issue is not a locked wallet -- I think this is ready to be merged.

@droark
Copy link
Contributor Author

droark commented Nov 16, 2015

Oh no, it's totally fine! I'm happy to make any and all changes as needed. I'll look at everything and update in a little bit.

@droark
Copy link
Contributor Author

droark commented Nov 18, 2015

@gwillen - I believe I've addressed your feedback. Forgot to run a sanity check but I'll do that right now. Anything else you'd like for me to do? Should I squash the new commit?

EDIT: Sanity check was fine.

@gwillen
Copy link
Contributor

gwillen commented Nov 18, 2015

Looks good, thanks! Go ahead and squash now that I've looked at it. EDIT: And I've figured out the process so I am ready to merge this once you squash it.

- Add support for command line arguments to the sidechain manipulation script. argparse requires Python 2.7 or 3.2+.
- Add support for unlocking mainchain wallet when running send-to-sidechain.
- Add various bits of help text, some of which may not be quite right.
@droark
Copy link
Contributor Author

droark commented Nov 18, 2015

squash

@gwillen gwillen merged commit 505a6d1 into ElementsProject:alpha Nov 18, 2015
gwillen added a commit that referenced this pull request Nov 18, 2015
505a6d1 Add CL arg support to sidechain manipulation script (Douglas Roark)
@gwillen
Copy link
Contributor

gwillen commented Nov 18, 2015

Ok, after a bit of a battle with the commit-signing script, it is done! Thanks for your patience and thanks for your contribution!

@droark
Copy link
Contributor Author

droark commented Nov 18, 2015

Thanks! I'll keep an eye out for any more contributions I can make.

instagibbs added a commit to instagibbs/elements that referenced this pull request Mar 15, 2017
FundTransaction: better estimate final tx size
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants