Conversation
Codecov Report
@@ Coverage Diff @@
## v0.11.0-dev #1793 +/- ##
===============================================
+ Coverage 58.71% 62.00% +3.29%
===============================================
Files 582 596 +14
Lines 22924 26766 +3842
===============================================
+ Hits 13459 16596 +3137
- Misses 7318 7924 +606
- Partials 2147 2246 +99
Continue to review full report at Codecov.
|
svarogg
left a comment
svarogg
left a comment
There was a problem hiding this comment.
I've only reviewed the first test properly, the rest of the tests have similar problems, so please fix all of them.
Also, please try to find as much common code between the tests and extract that into methods.
| } | ||
| defer teardown(false) | ||
|
|
||
| blockAHash, _, err := testConsensus.AddBlock([]*externalapi.DomainHash{testConsensus.DAGParams().GenesisHash}, nil, nil) |
There was a problem hiding this comment.
Please add a comment explaining the structure of DAG you are building, and why
| } | ||
| fees := uint64(1) | ||
| // Create a CSV script | ||
| numOfDAAScoreToWait := uint64(10) |
There was a problem hiding this comment.
Please rename num -> number
There was a problem hiding this comment.
Alternatively, you can just rename to daaScoreToWait
| @@ -0,0 +1,524 @@ | |||
| package consensus_test | |||
There was a problem hiding this comment.
The filename should be all_lower_case (same for timelock_cltv_test.go)
| if err != nil { | ||
| t.Fatalf("Error creating blockE: %v", err) | ||
| } | ||
| // bit 62 of sequence defines if it's conditioned by DAA score(set to 0) or by time (set to 1). |
There was a problem hiding this comment.
These are implementation details that you are leaking outside where they are supposed to be.
To mitigate, do one of the following:
- Either convert sequenceTypeFlag to boolean.
- Or do some magic with
constants.SequenceLockTimeIsSeconds. I don't have a concrete plan what to do, discuss with me if you prefer this path/
| if err != nil { | ||
| t.Fatalf("Error creating transactionThatSpentTheLockedOutput: %v", err) | ||
| } | ||
| // Add a block that contains a transaction that spends the locked output before the time, and therefore should be failed. |
There was a problem hiding this comment.
Not failed, but rather - rejected
| if err != nil { | ||
| t.Fatalf("The block should be valid since the output is not locked anymore. but got an error: %v", err) | ||
| } | ||
| validBlockStatus, err := testConsensus.BlockStatusStore().Get(testConsensus.DatabaseContext(), stagingArea, |
There was a problem hiding this comment.
It's not valid yet. It's only expected to be valid.
Please rename to actualBlockStatus or just blockStatus
| if err != nil { | ||
| t.Fatalf("Error creating blockA: %v", err) | ||
| } | ||
| blockBHash, _, err := testConsensus.AddBlock([]*externalapi.DomainHash{blockAHash}, nil, nil) |
There was a problem hiding this comment.
You are creating 4 blocks one after the other. Please use a loop
| } | ||
|
|
||
| // TestRelativeTimeOnCheckSequenceVerify verifies that if the relative target is set to be X DAA score, | ||
| // and the absolute DAA score is X before having X DAA score more than the time the UTXO was mined, then the output will remain locked. |
There was a problem hiding this comment.
and the absolute DAA score is
There's no such things as "the absolute DAA score", there's "the actual DAA score" or something similar.
There was a problem hiding this comment.
Also the rest of the sentance is really hard to understand, please revise (Talk to me if need someone to brainstorm with)
| if err != nil { | ||
| return nil, err | ||
| } | ||
| stamp := uint64(lockedOutputBlock.Header.TimeInMilliseconds()) |
There was a problem hiding this comment.
Please rename to timestamp
| testConsensus *testapi.TestConsensus) (*externalapi.DomainTransaction, error) { | ||
|
|
||
| var lockTime, sequence uint64 | ||
| if sequenceTypeFlag == 1 { // Conditioned by time: |
There was a problem hiding this comment.
Please take as much lines as possible out of the if
754c80d to
cc52481
Compare
|
is this an important test, or why is it still in the PRs while being so old? |
3 participants
The second part of #1257
Tests for CSV (conditioned by time and DAA score).