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.

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

Remove instantiable Address #401

Merged
merged 7 commits into from Jul 28, 2015
Merged

Remove instantiable Address #401

merged 7 commits into from Jul 28, 2015

Conversation

dcousens
Copy link
Contributor

WIP pull request. Ready to merge

@dcousens dcousens added the todo label Apr 28, 2015
@dcousens dcousens self-assigned this Apr 28, 2015
@dcousens dcousens added this to the 2.0.0 milestone Apr 28, 2015
weilu
weilu reviewed May 3, 2015
View reviewed changes
src/address.js

assert(false, this.toString() + ' has no matching Script')
} catch (e) {
throw new Error(address + ' is not a valid ' + network + ' address')
Copy link
Contributor

Choose a reason for hiding this comment

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

Does validate have to throw on invalid? Consider returning a boolean instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

@weilu
Copy link
Contributor

weilu commented Jul 5, 2015

@dcousens Let's finish this.

@dcousens
Copy link
Contributor Author

dcousens commented Jul 7, 2015

@weilu sounds good, I'll get back on this today.

@dcousens
Copy link
Contributor Author

Integration tests/coveralls failing for unrelated reasons.

@weilu please review.
Should be quickest to review commit-by-commit.

The API could maybe change, but lets do that in a different PR, I want to get this merged and leave the API bike shedding separate.

@jprichardson feel free to review also. :)

@dcousens
Copy link
Contributor Author

Intend to merge ASAP. Any comments/qualms?

@jprichardson
Copy link
Member

Most of it looks good. I like this change. My only thought is not removing the tests (you'd still keep this commit as is 16a711b i.e. remove these tests as you have). But by not removing the other tests and maybe just changing them to errors helps to communicate how the expectations have changed. I'm fine with it as is, just a thought.

Also, what's with bundling the commit to add the network into TxBuilder into this PR?

@dcousens dcousens force-pushed the noaddr branch 2 times, most recently from fe63cb3 to 1a4000c Compare July 24, 2015 02:20
@dcousens
Copy link
Contributor Author

@jprichardson I rebased without that commit and will submit in a different PR. Thanks for the spot.

Please merge if happy.

@dcousens dcousens mentioned this pull request Jul 24, 2015
Merged
weilu
weilu reviewed Jul 26, 2015
View reviewed changes
src/address.js
var version = payload.readUInt8(0)
var hash = payload.slice(1)

return new Address(hash, version)
return { hash: hash, version: version }
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a big fan of the return value of this function. How about splitting this into two functions: hashFromBase58Check and versionFromBase58Check?

@weilu
Copy link
Contributor

weilu commented Jul 26, 2015

There will be a PR for adding deprecation warnings on 1.x branch right?

@dcousens
Copy link
Contributor Author

fromBase58Check is temporary IMHO.
It shouldn't make 2.0.0, but I'll remove it in another PR as its a topic on its own.

There will be a PR for adding deprecation warnings on 1.x branch right?

Of course.

Add a possibleNetworks here as the 2nd argument, and pass it to findNetworkByWIFVersion for network detection.

I think that's worth investigating, but we need to do consistently this across all the network sections, and I'd be happy to do that in #425.

@dcousens
Copy link
Contributor Author

Going to merge, as the issues brought up (here and privately) were all expected:

  • fromBase58check feels like an internal API, but it still useful, lets find a decent replacement (PR to come).
  • Network detection cleanup (will be addressed in Remove network detection #425)
  • Deprecation warnings (will put a note on the 1.x branch for pre-2.0.0 tasks) (added in 1.x #376 (comment))

dcousens added a commit that referenced this pull request Jul 28, 2015
@dcousens
Merge pull request #401 from bitcoinjs/noaddr
64f7fa0 
Remove instantiable Address
@dcousens dcousens merged commit 64f7fa0 into master Jul 28, 2015
@dcousens dcousens deleted the noaddr branch July 28, 2015 00:46
@dcousens dcousens mentioned this pull request Jul 28, 2015
Closed
@coveralls
Copy link

Coverage Status

Coverage decreased (-14.5%) to 84.923% when pulling 1a4000c on noaddr into 5ce0937 on master.

@dcousens
Copy link
Contributor Author

@coveralls, you trippin

@Ashtonulliac Ashtonulliac mentioned this pull request Aug 1, 2019
Merged
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.

None yet
4 participants
@dcousens @weilu @jprichardson @coveralls