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

txbuilder: add setLockTime #507

Merged
merged 3 commits into from
Jan 27, 2016
Merged

txbuilder: add setLockTime #507

merged 3 commits into from
Jan 27, 2016

Conversation

dcousens
Copy link
Contributor

WIP, needs tests.

Open to feedback, not the biggest fan, but, its simple enough.

Maybe I should use a property instead?

@dcousens dcousens self-assigned this Nov 26, 2015
@dcousens
Copy link
Contributor Author

Resolves #437

@dcousens
Copy link
Contributor Author

ping @jprichardson thoughts on API?

@jprichardson
Copy link
Member

setLockTime() is fine.

An aside, builders are commonly used in conjunction with a fluent interface. Since it doesn't, what's the rationalization for splitting up Transaction / TransactionBuilder? (Really doesn't have to be answered here)

@dcousens
Copy link
Contributor Author

Transaction is just the data structure, no state, capable of being manually mutated and it will still serialize correctly.
To which, yes, it probably does make sense to remove Transaction.add* on it and just have transaction.outputs.push etc.

@dcousens
Copy link
Contributor Author

An aside, builders are commonly used in conjunction with a fluent interface.

Any ideas on how we could improve this interface?
I'm not opposed to a chain-based/simpler API for 3.0.0.

The requirements are just that I'd like to not enforce that all private keys must be available when signing.


return input.signatures.some(function (s) { return s })
})) {
throw new Error('No, this would invalidate signatures')
Copy link
Contributor

Choose a reason for hiding this comment

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

I still find this error message funny because it answers "no" without any question asked :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha, I know. But, it works.
If you can think of something for 3.0.0, go for it :)

@weilu
Copy link
Contributor

weilu commented Dec 13, 2015

The API looks ok to me.

@dcousens dcousens changed the title txbuilder: add setLockTime txbuilder: add setLockTime [WIP] Dec 21, 2015
@dcousens dcousens changed the title txbuilder: add setLockTime [WIP] txbuilder: add setLockTime Jan 27, 2016
@dcousens dcousens assigned jprichardson and unassigned dcousens Jan 27, 2016
@dcousens
Copy link
Contributor Author

@jprichardson not going to merge this without status checks as tests have been added.
Please verify manually and merge when ready. 👍

@dcousens
Copy link
Contributor Author

Needed #524, so merged manually after local verification.
@jprichardson please review, verify and merge.

@dcousens dcousens modified the milestone: 2.2.0 Jan 27, 2016
jprichardson added a commit that referenced this pull request Jan 27, 2016
@jprichardson jprichardson merged commit 4ee194e into master Jan 27, 2016
@jprichardson
Copy link
Member

👍

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 9a62ab6 on locktime into ** on master**.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants