Tests check sequence verify #1793
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.
|
| } | ||
| 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
someone235
force-pushed
the
v0.11.1-dev
branch
from
754c80d
to
cc52481
Compare
|
is this an important test, or why is it still in the PRs while being so old? |
The second part of #1257
Tests for CSV (conditioned by time and DAA score).