[bisq-network/bisq-core] Refactoring TxParser (#170)

chirhonul notifications at github.com
Mon Aug 27 17:17:46 UTC 2018


chirhonul approved this pull request.

utACK 5e77af61.

See some nits and optional comments inline.

In 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.

> @@ -67,6 +67,7 @@
         this.address = address;
         this.opReturnData = opReturnData;
         this.blockHeight = blockHeight;
+

(Optional) Seems like this newline was added accidentally?

> @@ -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.

(Nit) This comment seems like it should be a docstring?

> @@ -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

s/`hte`/`the`

>                  ",\n     unlockBlockHeight=" + unlockBlockHeight +
                 "\n} " + super.toString();
     }
+

(Optional) Seems like this newline was added accidentally?

> @@ -287,20 +288,24 @@ public MeritList getMerits(@Nullable String blindVoteTxId) {
                     if (blindVoteTxId != null) {
                         String pubKey = issuance.getPubKey();
                         if (pubKey == null) {
+                            // Maybe add exception

(Nit) This comment seems like it should be a `TODO`?

>                              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

(Nit) This comment seems like it should be a `TODO`?

> @@ -81,6 +76,7 @@ public static MeritList getDecryptMeritList(byte[] encryptedMeritList, SecretKey
         }
     }
 
+    // TODO move ot MeritConsensus

Nit: s/`ot`/`to`

> @@ -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

Some of these `TODO`s seems like they would be easy to address in this PR, but in discussion with @ManfredKarrer he prefers to merge as-is and send follow-up PRs to remove these now unnecessary `OP_RETURN` helper classes.

> +        // 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;

Nit: Could do `return isFeeCorrect` on this line and remove the `return false` on L267.

> +
+            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;

Nit: Could do `return isInPhase` here and remove the `return false` on L279.

> @@ -67,6 +67,7 @@
         this.address = address;
         this.opReturnData = opReturnData;
         this.blockHeight = blockHeight;
+

(Optional) This newline seems like it was added by accident?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/bisq-network/bisq-core/pull/170#pullrequestreview-149796330
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20180827/8849a131/attachment.html>


More information about the bisq-github mailing list