[bisq-network/bisq] Dao improvements (#1746)

[sqrrm]

sqrrm approved this pull request.

utACK

Some good refactoring making it much clearer I think. As noticed you fixed all the issues found along the way.

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

Comment can be removed

> @@ -52,7 +52,8 @@
 
     private final BsqStateService bsqStateService;
     @Getter
-    private TxInputParser.VoteRevealInputState voteRevealInputState = TxInputParser.VoteRevealInputState.UNKNOWN;
+    boolean isVoteRevealInputInValid = false;

This should be isVoteRevealInputValid

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

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.

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

This comment is quite hard to understand.

>                              // 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()) {

I see the naming got fixed, good :+1: 

> @@ -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()) {

I think this is now incorrect, it should be `!txInputParser.isVoteRevealInputValid()`

>       */
     public int getLockTime() {
         return txOutputs.get(0).getLockTime();
     }
 
+    /**
+     * The unlockBlockHeight is stored in the LOCKUP txOutput, which is the first txOutput.

I think that should be `... stored in the UNLOCK txOutput ...` 

> @@ -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()) {

You fixed it!

-- 
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/pull/1746#pullrequestreview-161495710
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20181004/1ab65eb4/attachment.html>