<p><b>@chirhonul</b> approved this pull request.</p>
<p>utACK <a class="commit-link" href="https://github.com/bisq-network/bisq-core/commit/5e77af618e6d62c02b9fcd8c03981f6bef5f7e58"><tt>5e77af6</tt></a>.</p>
<p>See some nits and optional comments inline.</p>
<p>In commit c55778 we change rather a lot of files, and most of the edits is to add various <code>TODO</code>s.. in discussion with <a class="user-mention" data-hovercard-user-id="1449498" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://github.com/ManfredKarrer">@ManfredKarrer</a> it seems that the commit was accidentally added to the branch. But given that the merge to a monorepo will likely lose the commit history anyway, and the changes do seem safe, merging them in the current PR might be the best way to proceed.</p><hr>
<p>In <a href="https://github.com/bisq-network/bisq-core/pull/170#discussion_r213044164">src/main/java/bisq/core/dao/state/blockchain/BaseTxOutput.java</a>:</p>
<pre style='color:#555'>> @@ -67,6 +67,7 @@
this.address = address;
this.opReturnData = opReturnData;
this.blockHeight = blockHeight;
+
</pre>
<p>(Optional) Seems like this newline was added accidentally?</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq-core/pull/170#discussion_r213044258">src/main/java/bisq/core/dao/state/blockchain/Tx.java</a>:</p>
<pre style='color:#555'>> @@ -142,14 +136,20 @@ public TxOutput getLastTxOutput() {
return txOutputs.get(txOutputs.size() - 1);
}
+ // OpReturn output might contain the lockTime in case of a LockTx. It has to be the last output.
</pre>
<p>(Nit) This comment seems like it should be a docstring?</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq-core/pull/170#discussion_r213044327">src/main/java/bisq/core/dao/state/blockchain/Tx.java</a>:</p>
<pre style='color:#555'>> @@ -142,14 +136,20 @@ public TxOutput getLastTxOutput() {
return txOutputs.get(txOutputs.size() - 1);
}
+ // OpReturn output might contain the lockTime in case of a LockTx. It has to be the last output.
+ // We store technically hte lockTime there as is is stored in the OpReturn data but conceptually we want to provide
</pre>
<p>s/<code>hte</code>/<code>the</code></p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq-core/pull/170#discussion_r213044499">src/main/java/bisq/core/dao/state/blockchain/Tx.java</a>:</p>
<pre style='color:#555'>> ",\n unlockBlockHeight=" + unlockBlockHeight +
"\n} " + super.toString();
}
+
</pre>
<p>(Optional) Seems like this newline was added accidentally?</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq-core/pull/170#discussion_r213045442">src/main/java/bisq/core/dao/governance/blindvote/MyBlindVoteListService.java</a>:</p>
<pre style='color:#555'>> @@ -287,20 +288,24 @@ public MeritList getMerits(@Nullable String blindVoteTxId) {
if (blindVoteTxId != null) {
String pubKey = issuance.getPubKey();
if (pubKey == null) {
+ // Maybe add exception
</pre>
<p>(Nit) This comment seems like it should be a <code>TODO</code>?</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq-core/pull/170#discussion_r213045465">src/main/java/bisq/core/dao/governance/blindvote/MyBlindVoteListService.java</a>:</p>
<pre style='color:#555'>> log.error("We did not have a pubKey in our issuance object. " +
"txId={}, issuance={}", issuance.getTxId(), issuance);
return null;
}
DeterministicKey key = bsqWalletService.findKeyFromPubKey(Utilities.decodeFromHex(pubKey));
if (key == null) {
+ // Maybe add exception
</pre>
<p>(Nit) This comment seems like it should be a <code>TODO</code>?</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq-core/pull/170#discussion_r213045690">src/main/java/bisq/core/dao/governance/voteresult/VoteResultConsensus.java</a>:</p>
<pre style='color:#555'>> @@ -81,6 +76,7 @@ public static MeritList getDecryptMeritList(byte[] encryptedMeritList, SecretKey
}
}
+ // TODO move ot MeritConsensus
</pre>
<p>Nit: s/<code>ot</code>/<code>to</code></p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq-core/pull/170#discussion_r213046010">src/main/java/bisq/core/dao/node/parser/OpReturnBlindVoteParser.java</a>:</p>
<pre style='color:#555'>> @@ -44,6 +44,7 @@ public OpReturnBlindVoteParser(PeriodService periodService,
// We do not check the version as if we upgrade the a new version old clients would fail. Rather we need to make
// a change backward compatible so that new clients can handle both versions and old clients are tolerant.
boolean validate(byte[] opReturnData, long bsqFee, int blockHeight, ParsingModel parsingModel) {
+ //TODO remove
</pre>
<p>Some of these <code>TODO</code>s seems like they would be easy to address in this PR, but in discussion with <a class="user-mention" data-hovercard-user-id="1449498" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://github.com/ManfredKarrer">@ManfredKarrer</a> he prefers to merge as-is and send follow-up PRs to remove these now unnecessary <code>OP_RETURN</code> helper classes.</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq-core/pull/170#discussion_r213046359">src/main/java/bisq/core/dao/node/parser/TxParser.java</a>:</p>
<pre style='color:#555'>> + // The leftover BSQ balance from the inputs is the BSQ fee in case we are in an OP_RETURN output
+
+ if (!isPhaseValid(blockHeight, tempTx, phase))
+ return false;
+
+ long paramValue = bsqStateService.getParamValue(param, blockHeight);
+ boolean isFeeCorrect = bsqFee == paramValue;
+ if (!isFeeCorrect) {
+ log.warn("Invalid fee. used fee={}, required fee={}", bsqFee, paramValue);
+ tempTx.setTxType(TxType.INVALID);
+ // TODO should we return already?
+
+ return false;
+ }
+
+ return true;
</pre>
<p>Nit: Could do <code>return isFeeCorrect</code> on this line and remove the <code>return false</code> on L267.</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq-core/pull/170#discussion_r213046492">src/main/java/bisq/core/dao/node/parser/TxParser.java</a>:</p>
<pre style='color:#555'>> +
+ return false;
+ }
+
+ return true;
+ }
+
+ private boolean isPhaseValid(int blockHeight, TempTx tempTx, DaoPhase.Phase phase) {
+ boolean isInPhase = periodService.isInPhase(blockHeight, phase);
+ if (!isInPhase) {
+ log.warn("Not in {} phase. blockHeight={}", phase, blockHeight);
+ tempTx.setTxType(TxType.INVALID);
+ // TODO should we return already?
+ return false;
+ }
+ return true;
</pre>
<p>Nit: Could do <code>return isInPhase</code> here and remove the <code>return false</code> on L279.</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq-core/pull/170#discussion_r213046568">src/main/java/bisq/core/dao/state/blockchain/BaseTxOutput.java</a>:</p>
<pre style='color:#555'>> @@ -67,6 +67,7 @@
this.address = address;
this.opReturnData = opReturnData;
this.blockHeight = blockHeight;
+
</pre>
<p>(Optional) This newline seems like it was added by accident?</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-core/pull/170#pullrequestreview-149796330">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AkpZtudtaYFa64A_E9_VylGPm_GK9YAFks5uVCm6gaJpZM4WOFqH">mute the thread</a>.<img src="https://github.com/notifications/beacon/AkpZtiOIwiKswSfuz7USw9GomT75jVSZks5uVCm6gaJpZM4WOFqH.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-core","title":"bisq-network/bisq-core","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-core"}},"updates":{"snippets":[{"icon":"PERSON","message":"@chirhonul approved #170"}],"action":{"name":"View Pull Request","url":"https://github.com/bisq-network/bisq-core/pull/170#pullrequestreview-149796330"}}}</script>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/bisq-network/bisq-core/pull/170#pullrequestreview-149796330",
"url": "https://github.com/bisq-network/bisq-core/pull/170#pullrequestreview-149796330",
"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": "@chirhonul approved 170",
"sections": [
{
"text": "utACK 5e77af61.\r\n\r\nSee some nits and optional comments inline.\r\n\r\nIn commit c55778 we change rather a lot of files, and most of the edits is to add various `TODO`s.. in discussion with @ManfredKarrer it seems that the commit was accidentally added to the branch. But given that the merge to a monorepo will likely lose the commit history anyway, and the changes do seem safe, merging them in the current PR might be the best way to proceed.",
"activityTitle": "**chirhonul**",
"activityImage": "https://assets-cdn.github.com/images/email/message_cards/avatar.png",
"activitySubtitle": "@chirhonul",
"facts": [
]
}
],
"potentialAction": [
{
"targets": [
{
"os": "default",
"uri": "https://github.com/bisq-network/bisq-core/pull/170#pullrequestreview-149796330"
}
],
"@type": "OpenUri",
"name": "View on GitHub"
},
{
"name": "Unsubscribe",
"@type": "HttpPOST",
"target": "https://api.github.com",
"body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 372791943\n}"
}
],
"themeColor": "26292E"
}
]</script>