Add serialization for btc tx to avoid passing boolean, and reuse it when possible #2734
Conversation
| private static BtcTransaction deserializeBtcTransactionFromRLPData(byte[] rlpData, NetworkParameters networkParameters, boolean txHasInputs) { | ||
| if (!txHasInputs) { | ||
| BtcTransaction tx = new BtcTransaction(networkParameters); | ||
| tx.parseNoInputs(rlpData); | ||
| return tx; | ||
| } | ||
|
|
||
| return new BtcTransaction(networkParameters, rlpData); | ||
| } |
There was a problem hiding this comment.
| private static BtcTransaction deserializeBtcTransactionFromRLPData(byte[] rlpData, NetworkParameters networkParameters, boolean txHasInputs) { | |
| if (!txHasInputs) { | |
| BtcTransaction tx = new BtcTransaction(networkParameters); | |
| tx.parseNoInputs(rlpData); | |
| return tx; | |
| } | |
| return new BtcTransaction(networkParameters, rlpData); | |
| } | |
| private static BtcTransaction deserializeBtcTransactionFromRLPData( | |
| byte[] rlpData, NetworkParameters networkParameters, boolean hasInputs) { | |
| if (hasInputs) { | |
| return new BtcTransaction(networkParameters, rlpData); | |
| } | |
| BtcTransaction tx = new BtcTransaction(networkParameters); | |
| tx.parseNoInputs(rlpData); | |
| return tx; | |
| } |
There was a problem hiding this comment.
i prefer to have the less used case inside the if, to improve readability.
(Currently, we only use this deserialization having inputs)
| public static byte[] serializeRskTxsWaitingForSignatures( | ||
| SortedMap<Keccak256, BtcTransaction> rskTxWaitingForSignaturesMap) { | ||
| public static byte[] serializeRskTxsWaitingForSignatures(SortedMap<Keccak256, BtcTransaction> rskTxWaitingForSignaturesMap) { |
There was a problem hiding this comment.
Google Java Style Guide enforces a 100-character line limit, is there any reason why this is being changed across the class?
| public static BtcTransaction deserializeSvpFundTx(byte[] data, NetworkParameters networkParameters) { | ||
| return deserializeBtcTransaction(data, networkParameters, true); | ||
| } |
There was a problem hiding this comment.
Do we need a serialize method that complements this one?
|
|
||
| // Act | ||
| byte[] serializedSvpFundTransaction = BridgeSerializationUtils.serializeBtcTransaction(svpFundTx); | ||
| BtcTransaction deserializedSvpFundTransaction = BridgeSerializationUtils.deserializeSvpFundTx(serializedSvpFundTransaction, NETWORK_PARAMETERS); |
There was a problem hiding this comment.
Not sure if we really need this method that simply acts as a wrapper. Wdyt?
There was a problem hiding this comment.
thats one of the points of this refactor, having specific methods when deserializing txs we know they have inputs to avoid passing the boolean.
We need to keep the whole txHasInputs thing since its part of bitcoin, but is not the cleanest thing to do 😄
| @Test | ||
| void serializeAndDeserializeBtcTransaction_withValidDataAndInputs_shouldReturnEqualResults() { | ||
| // Arrange | ||
| BtcTransaction fundTx = new BtcTransaction(NETWORK_PARAMETERS); |
There was a problem hiding this comment.
fundTx might give the impression this is something related to svp, what about we call it prevTx here as well
| int n = 0; | ||
|
|
||
| int n = 0; |
There was a problem hiding this comment.
this was moved to be closer from where its being used
… avoid passing boolean
…blank lines to improve readability
…thods to deserialize btc txs with and without inputs.
marcos-iov
force-pushed
the
feat/refactor_bridge_serialization_utils
branch
from
5a1e6c4 to
44c4644
Compare
|
marcos-iov
merged commit 98ed61b
into
feature/powpeg_validation_protocol-phase2
Also add serialization for rsk tx hash to reuse it when possible