<p><b>@sqrrm</b> approved this pull request.</p>
<p>utACK</p>
<p>Some good refactoring making it much clearer I think. As noticed you fixed all the issues found along the way.</p><hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/1746#discussion_r222577727">core/src/main/java/bisq/core/dao/node/parser/TxParser.java</a>:</p>
<pre style='color:#555'>> @@ -127,7 +127,7 @@ public TxParser(PeriodService periodService,
int lastNonOpReturnIndex = lastIndex;
if (txOutputParser.isOpReturnOutput(outputs.get(lastIndex))) {
// TODO(SQ): perhaps the check for isLastOutput could be skipped
</pre>
<p>Comment can be removed</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/1746#discussion_r222580000">core/src/main/java/bisq/core/dao/node/parser/TxInputParser.java</a>:</p>
<pre style='color:#555'>> @@ -52,7 +52,8 @@
private final BsqStateService bsqStateService;
@Getter
- private TxInputParser.VoteRevealInputState voteRevealInputState = TxInputParser.VoteRevealInputState.UNKNOWN;
+ boolean isVoteRevealInputInValid = false;
</pre>
<p>This should be isVoteRevealInputValid</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/1746#discussion_r222712251">core/src/main/java/bisq/core/dao/node/parser/TxOutputParser.java</a>:</p>
<pre style='color:#555'>> boolean isOpReturnOutput(TempTxOutput txOutput) {
return txOutput.getOpReturnData() != null;
}
void processOpReturnOutput(TempTxOutput tempTxOutput) {
byte[] opReturnData = tempTxOutput.getOpReturnData();
- if (opReturnData != null) {
- handleOpReturnOutput(tempTxOutput);
- } else {
- log.error("This should be an opReturn output");
- }
+ checkNotNull(opReturnData, "opReturnData must not be null");
+ TxOutputType txOutputType = OpReturnParser.getTxOutputType(tempTxOutput);
+ tempTxOutput.setTxOutputType(txOutputType);
+
+ optionalOpReturnType = getMappedOpReturnType(txOutputType);
+ // TODO: Do we really want to store the lockTime in another output as where we get it delivered?
</pre>
<p>Since the lockTime is referring to the lockTime of the first output I think it's correct. It just happens that the information has to be carried in the opreturn, and this is when it's parsed and assigned to the concerned txOutput.</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/1746#discussion_r222721688">core/src/main/java/bisq/core/dao/node/parser/TxParser.java</a>:</p>
<pre style='color:#555'>> @@ -318,50 +337,49 @@ private boolean isPhaseValid(int blockHeight, DaoPhase.Phase phase) {
/**
* Retrieve the type of the transaction, assuming it is relevant to bisq.
*
- * @param tx The temporary transaction.
- * @param hasOpReturnCandidate True if we have a candidate for an OP_RETURN.
- * @param remainingInputValue The remaining value of inputs not yet accounted for, in satoshi.
- * @param optionalOpReturnType If present, the OP_RETURN type of the transaction.
+ * @param tempTx The temporary transaction.
+ * @param optionalOpReturnType The optional OP_RETURN type of the transaction.
+ * @param hasBurntBSQ If the have been remaining value from the inputs which got not spent in outputs.
</pre>
<p>This comment is quite hard to understand.</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/1746#discussion_r222723842">core/src/main/java/bisq/core/dao/node/parser/TxInputParser.java</a>:</p>
<pre style='color:#555'>> // The connected tx output of the blind vote tx is our input for the reveal tx.
// We allow only one input from any blind vote tx otherwise the vote reveal tx is invalid.
- if (!isVoteRevealInputInValid) {
+ if (!isVoteRevealInputValid()) {
</pre>
<p>I see the naming got fixed, good <g-emoji class="g-emoji" alias="+1" fallback-src="https://assets-cdn.github.com/images/icons/emoji/unicode/1f44d.png">👍</g-emoji></p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/1746#discussion_r222729289">core/src/main/java/bisq/core/dao/node/parser/TxParser.java</a>:</p>
<pre style='color:#555'>> @@ -293,7 +291,7 @@ private void processVoteReveal(int blockHeight, TempTx tempTx) {
}
// We must not use an `if else` here!
- if (!isPhaseValid || txInputParser.isVoteRevealInputInValid()) {
+ if (!isPhaseValid || txInputParser.isVoteRevealInputValid()) {
</pre>
<p>I think this is now incorrect, it should be <code>!txInputParser.isVoteRevealInputValid()</code></p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/1746#discussion_r222731311">core/src/main/java/bisq/core/dao/state/blockchain/Tx.java</a>:</p>
<pre style='color:#555'>> */
public int getLockTime() {
return txOutputs.get(0).getLockTime();
}
+ /**
+ * The unlockBlockHeight is stored in the LOCKUP txOutput, which is the first txOutput.
</pre>
<p>I think that should be <code>... stored in the UNLOCK txOutput ...</code></p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/1746#discussion_r222731711">core/src/main/java/bisq/core/dao/node/parser/TxParser.java</a>:</p>
<pre style='color:#555'>> @@ -291,7 +291,7 @@ private void processVoteReveal(int blockHeight, TempTx tempTx) {
}
// We must not use an `if else` here!
- if (!isPhaseValid || txInputParser.isVoteRevealInputValid()) {
+ if (!isPhaseValid || !txInputParser.isVoteRevealInputValid()) {
</pre>
<p>You fixed it!</p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/bisq-network/bisq/pull/1746#pullrequestreview-161495710">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AkpZtrYY_ZuumkOI-oEtpUz13Dwk-m_Mks5uhje1gaJpZM4XHaO5">mute the thread</a>.<img src="https://github.com/notifications/beacon/AkpZtjNMulzoWlqeScdAEie0pSNE6k-hks5uhje1gaJpZM4XHaO5.gif" height="1" width="1" alt="" /></p>
<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/bisq-network/bisq","title":"bisq-network/bisq","subtitle":"GitHub repository","main_image_url":"https://assets-cdn.github.com/images/email/message_cards/header.png","avatar_image_url":"https://assets-cdn.github.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/bisq-network/bisq"}},"updates":{"snippets":[{"icon":"PERSON","message":"@sqrrm approved #1746"}],"action":{"name":"View Pull Request","url":"https://github.com/bisq-network/bisq/pull/1746#pullrequestreview-161495710"}}}</script>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/bisq-network/bisq/pull/1746#pullrequestreview-161495710",
"url": "https://github.com/bisq-network/bisq/pull/1746#pullrequestreview-161495710",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
},
{
"@type": "MessageCard",
"@context": "http://schema.org/extensions",
"hideOriginalBody": "false",
"originator": "AF6C5A86-E920-430C-9C59-A73278B5EFEB",
"title": "@sqrrm approved 1746",
"sections": [
{
"text": "utACK\r\n\r\nSome good refactoring making it much clearer I think. As noticed you fixed all the issues found along the way.",
"activityTitle": "**sqrrm**",
"activityImage": "https://assets-cdn.github.com/images/email/message_cards/avatar.png",
"activitySubtitle": "@sqrrm",
"facts": [
]
}
],
"potentialAction": [
{
"targets": [
{
"os": "default",
"uri": "https://github.com/bisq-network/bisq/pull/1746#pullrequestreview-161495710"
}
],
"@type": "OpenUri",
"name": "View on GitHub"
},
{
"name": "Unsubscribe",
"@type": "HttpPOST",
"target": "https://api.github.com",
"body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 387818425\n}"
}
],
"themeColor": "26292E"
}
]</script>