Skip to content

Conversation

@dcousens
Copy link
Contributor

Depends on #139.

This pull request simplifies the Address module so that it conforms to a highly strict subset of its original functionality.
It is primarily a wrapper for a pubKeyHash or scriptHash; and is limited to that subset.

The following builder functions are supported:

  • Address.fromBase58Check, and
  • Address.fromPubKey

With the standard Address.prototype.toBase58Check being aliased by Address.prototype.toString.

This pull request introduces a slight circular dependency with ECPubKey, however the intent is to eventually remove ECKey.getAddress to resolve this.

Script.createOutputScript had to be modified for these changes to proceed, mostly due to its inability to support non-standard network versions.
The fix implemented for this is not very pretty, but it is a functioning replacement (no extra tests for this have been provided beyond the bitcoin network however, I do intend to do that when I refactor Transaction).

Test fixtures for Address have also been added in test/fixtures/address.
Unlike in other pull requests, I did not make an effort to first check compatibility with the original test vectors for this pull request, and instead migrated to the new test vectors immediately.
This was done because the new API was not immediately compatible with the old, and would have required all new tests anyway.
If requested, I can import the old test vectors for testing with the current test setup, however the current vectors should be more than suitable (verify if unsure).

@dcousens
Copy link
Contributor Author

I'm still not sure where the responsibility should lie for an Address, perhaps all we should be allowing is Address.fromScriptPubKey and Address.fromBase58Check.
As it stands at the moment, information is thrown away that could make other parts of the code much simpler.
I'll have to investigate this more eventually, for now, this should be a more acceptable stricter subset of the previous functionality.

Fixes #126 and conforms to #113 and #106 .

edit: accidentally closed the PR, re-opened

@dcousens dcousens closed this Apr 17, 2014
@dcousens dcousens reopened this Apr 17, 2014
@dcousens dcousens changed the title Migrates Address to Stricter API Migrates Address to stricter API Apr 17, 2014
@kyledrake
Copy link
Contributor

+1

@dcousens
Copy link
Contributor Author

Rebased on HEAD and added more meaningful error messages if the assertions fail.
Also handles the version parameter better by doing both a limit and type check.

@weilu
Copy link
Contributor

weilu commented Apr 18, 2014

@dcousens Can you rebase this one on top of current master? It'll be easier to review :)

@dcousens
Copy link
Contributor Author

Rebased on HEAD.

src/message.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. But address string is reasonable as an argument here. What about if(typeof address === 'string')?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to if(typeof address === 'string')? as it is more strict of what we are actually allowing as the alternative.

dcousens added a commit that referenced this pull request Apr 19, 2014
Migrates Address to stricter API
@dcousens dcousens merged commit c08f6a3 into bitcoinjs:master Apr 19, 2014
@dcousens dcousens deleted the addrstrict branch April 19, 2014 18:58
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