[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