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 setVersion #599

Merged
merged 1 commit into from
Jun 23, 2016
Merged

TxBuilder: add setVersion #599

merged 1 commit into from
Jun 23, 2016

Conversation

dcousens
Copy link
Contributor

I wasn't sure about this function at first, as it seemed so pass-through'ish (aka, perhaps a a sign of bad design).

But with BIP68 about to come into effect, it may be worth having some sanity checking logic here. We'll see.

(aka, setVersion(1) for a transaction the CSV OP codes is effectively invalid AFAIK)

@dcousens dcousens added this to the 2.3.0 milestone Jun 22, 2016
@dcousens dcousens self-assigned this Jun 22, 2016
@jprichardson
Copy link
Member

Would it be more appropriate to version the scripts instead of the tx?

@dcousens
Copy link
Contributor Author

@jprichardson well, no? The Transaction is what has a version, but, at least for CSV, it will be contents of the script which dictate what version of the transaction is required... so I'd say we just work within those constraints, if any of the scripts contain CSV, setVersion must have been set to 2, and we just maintain that invariant?

@jprichardson
Copy link
Member

so I'd say we just work within those constraints, if any of the scripts contain CSV, setVersion must have been set to 2, and we just maintain that invariant?

Yep, that sounds good.

LGTM.

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.

2 participants