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

Import settings from constants.py into sidechain-manipulation.py #86

Closed
wants to merge 1 commit into from

Conversation

TomMcCabe
Copy link

Avoids redundancies with changing the same settings in two separate files.

@martindale
Copy link

ACK.

TomMcCabe added a commit to TomMcCabe/elements that referenced this pull request Feb 29, 2016
Detail PR ElementsProject#86. Remove comment of Instagibbs fix as it was merged.
@gwillen
Copy link
Contributor

gwillen commented Mar 25, 2016

Oops, I was getting ready to merge this, but actually "secondScriptPubKey", removed from sidechain-manipulation.py by this change, is missing from constants.py. @TomMcCabe, can you fix, and double-check that nothing else is missing?

And can you please edit alpha-README.md to refer to editing constants.py instead of sidechain-manipulation.py, to match the code change (and fix any other documentation you're aware of, if any)?

Thanks!

(I think the reason this was not done this way to start with, was that 'fedpeg' was supposed to be a specialization of the general concept of sidechains, and so it doesn't make sense for this script to depend on something in 'fedpeg'. However, I think the removal of redundancy is more valuable than preserving that currently-theoretical separation, and we can always refactor it later.)

…echain-manipulation.py

045b310 Import settings from constants.py into sidechain-manipulation.py

b698a88 Restore original value of secondScriptPubKey to sidechain-manipulation.py

7474a1c Update alpha-README.md - edit RPC info in constants.py instead of sidechain-manipulation.py
@TomMcCabe
Copy link
Author

@gwillen: Fixed as requested and squashed commits.

@gwillen
Copy link
Contributor

gwillen commented May 28, 2016

Awesome, thanks and sorry for the delay. Taking a look now.

@gwillen
Copy link
Contributor

gwillen commented May 28, 2016

Lightly-tested ACK.

I don't know if there are conflicting views on this, but as a matter of personal preference, I tend to suggest waiting until after final review to squash (because it makes it easy for the reviewer to see what changed since the last review.)

I have rebased and will push.

@gwillen
Copy link
Contributor

gwillen commented May 28, 2016

Heh, github still claims this can be merged, probably because I did a rebase-push without updating the commit on the pullreq. But it's in! Thanks for doing this. :-)

@gwillen gwillen closed this May 28, 2016
TomMcCabe added a commit to TomMcCabe/elements that referenced this pull request Jul 12, 2016
The changes from this fetch-merge were merged in [ElementsProject#86](ElementsProject#86).
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.

3 participants