-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Transaction: add back 2 deprecated constructors #3510
base: release-0.17
Are you sure you want to change the base?
Transaction: add back 2 deprecated constructors #3510
Conversation
Move most of `Transaction.read` into a private constructor. Use this constructor to add back 2 removed constructors as deprecated constructors.
a89b6de
to
d64d663
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, except for the commented part that I could not verify yet. (Doesn't mean it's wrong.)
/** @deprecated use {@link Transaction#readOutputs(ByteBuffer)} */ | ||
@Deprecated | ||
public Transaction(NetworkParameters parameters, byte[] payload) { | ||
this(ByteBuffer.wrap(payload), ProtocolVersion.CURRENT.intValue()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if ProtocolVersion.CURRENT.intValue()
is always correct here. Did you look at how 0.16 deals with it? I just tried and sadly it's non-trivial.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should reduce the scope of this PR to just the refactoring that moves most of read()
into a private constructor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this be enough to cover the need of LDK? Then sure, the less changes the better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving most of read()
into a private constructor does not change the public API in any way, so it won't help LDK. It only makes any subsequent changes to constructors easier.
/** @deprecated use {@link Transaction#readOutputs(ByteBuffer)} */ | ||
@Deprecated | ||
public Transaction(NetworkParameters parameters, byte[] payload, int offset) { | ||
this(ByteBuffer.wrap(payload, offset, payload.length - offset), ProtocolVersion.CURRENT.intValue()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same.
Move most of
Transaction.read
into a private constructor. Use this constructor to add back 2 removed constructors as deprecated constructors.