Skip to content

Remove network detection #425

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 14 commits into from
Aug 7, 2015
Merged

Remove network detection #425

merged 14 commits into from
Aug 7, 2015
+204 −155

Conversation

@dcousens
Copy link
Contributor

@dcousens dcousens commented Jul 24, 2015

See #402 for discussion.

@dcousens dcousens added this to the 2.0.0 milestone Jul 24, 2015
@dcousens
Copy link
Contributor Author

dcousens commented Jul 24, 2015

This does not remove detection entirely, yet.

In case of detection of a network, require a list of possible networks passed in.

Any thoughts on how this should work with ECPair.fromWIF?
What would be the best succinct syntax for that?

@dcousens dcousens mentioned this pull request Jul 28, 2015
Merged
@dcousens dcousens assigned weilu and unassigned jprichardson Jul 28, 2015
dcousens
dcousens reviewed Jul 28, 2015
View reviewed changes
src/ecpair.js Outdated
Copy link
Contributor Author

@dcousens dcousens Jul 28, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@weilu this could probably be more efficient, but, it'll do for now

dcousens
dcousens reviewed Jul 28, 2015
View reviewed changes
src/ecpair.js
Copy link
Contributor Author

@dcousens dcousens Jul 28, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping @weilu

@dcousens dcousens mentioned this pull request Jul 28, 2015
Closed
weilu
weilu reviewed Aug 2, 2015
View reviewed changes
src/transaction_builder.js
Copy link
Contributor

@weilu weilu Aug 2, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a potential issue with this use: var cb = TransactionBuilder.fromTransaction always assumes bitcoin as the network. If that's called, followed by a call to cb.addOutput([a non-bitcoin address], value) without manually setting the network on cb it's no good.

Copy link
Contributor Author

@dcousens dcousens Aug 6, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, fromTransaction will need a network parameter also.

@weilu weilu assigned dcousens and unassigned weilu Aug 2, 2015
@weilu
Copy link
Contributor

weilu commented Aug 2, 2015

integration tests are passing locally. Not sure what's wrong with travis =/

@dcousens
Copy link
Contributor Author

dcousens commented Aug 7, 2015

Rebased and cherry picked 299cfce on top.

@dcousens
Copy link
Contributor Author

dcousens commented Aug 7, 2015

@weilu I addressed the above issues, looking to keep moving forward on this. Happy for any post reviews.

dcousens added a commit that referenced this pull request Aug 7, 2015
@dcousens
Merge pull request #425 from bitcoinjs/nonetdetect
1eb98be 
Remove network detection
@dcousens dcousens merged commit 1eb98be into master Aug 7, 2015
@dcousens dcousens deleted the nonetdetect branch August 7, 2015 10:29
@dcousens dcousens mentioned this pull request Aug 13, 2015
Closed
@weilu
Copy link
Contributor

weilu commented Aug 17, 2015

LGTM

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

Reviewers

No reviews

Assignees

@dcousens dcousens

Labels

breaking change

Projects

None yet

Milestone

2.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@dcousens @weilu @jprichardson