Add monotonic counter for submitted claims#479
Conversation
henriquemarlon
force-pushed
the
feat/claim-submission-counter
branch
2 times, most recently
from
16c1155 to
6c49df1
Compare
henriquemarlon
requested review from
Copilot and
guidanoli
and removed request for
guidanoli,
pedroargento and
vfusco
henriquemarlon
requested review from
guidanoli,
pedroargento and
vfusco
and removed request for
Copilot
| function testSubmittedClaimsCounter(bytes32[] calldata claims) public { | ||
| address owner = vm.addr(1); | ||
| address app = vm.addr(2); | ||
| uint256 epochLength = 5; | ||
|
|
||
| IAuthority authority = new Authority(owner, epochLength); | ||
|
|
||
| assertEq(authority.getNumberOfSubmittedClaims(), 0); | ||
|
|
||
| for (uint256 i; i < claims.length; ++i) { | ||
| uint256 blockNum = (i + 1) * epochLength; | ||
| vm.roll(blockNum); | ||
|
|
||
| vm.prank(owner); | ||
| authority.submitClaim(app, blockNum - 1, claims[i]); | ||
|
|
||
| assertEq(authority.getNumberOfSubmittedClaims(), i + 1); |
There was a problem hiding this comment.
this block seems redundant? The behaviour is already tested in line 136, right?
More tests can be added following how authority.getNumberOfAcceptedClaims() was tested and add one more line to test authority.getNumberOfSubmittedClaims() there :)
There was a problem hiding this comment.
You’re right @ZzzzHui I made the adjustment. Let me know if it meets what you proposed.
test/consensus/quorum/Quorum.t.sol
Outdated
| function testSubmittedClaimsCounter(bytes32[] calldata claims) external { | ||
| uint256 epochLength = 5; | ||
| uint256 numOfValidators = 3; | ||
|
|
||
| IQuorum quorum = _deployQuorum(numOfValidators, epochLength); | ||
|
|
||
| assertEq(quorum.getNumberOfSubmittedClaims(), 0); | ||
|
|
||
| Claim memory claim; | ||
| claim.appContract = vm.addr(1); | ||
|
|
||
| uint256 blockNum = epochLength; | ||
| vm.roll(blockNum); | ||
|
|
||
| uint256 totalSubmissions; | ||
|
|
||
| for (uint256 i = 0; i < claims.length; ++i) { | ||
| claim.lastProcessedBlockNumber = blockNum - 1; | ||
| claim.outputsMerkleRoot = claims[i]; | ||
|
|
||
| // submit claim with majority validators | ||
| uint256 majority = numOfValidators / 2 + 1; | ||
| for (uint256 id = 1; id <= majority; ++id) { | ||
| vm.prank(quorum.validatorById(id)); | ||
| quorum.submitClaim(claim); | ||
| ++totalSubmissions; | ||
| assertEq(quorum.getNumberOfSubmittedClaims(), totalSubmissions); | ||
| } | ||
|
|
||
| blockNum += epochLength; | ||
| vm.roll(blockNum); | ||
| } | ||
| } |
There was a problem hiding this comment.
since this is almost the same test as testMultipleClaimsAcceptedCounter, we could consider merging into it. We could then change the test name to something like testMultipleClaimsCounters etc
There was a problem hiding this comment.
I merged the tests under the name testMultipleClaimsCounters. Let me know if this looks better
henriquemarlon
force-pushed
the
feat/claim-submission-counter
branch
from
6c49df1 to
e13644e
Compare
henriquemarlon
force-pushed
the
feat/claim-submission-counter
branch
2 times, most recently
from
3210479 to
c8627c8
Compare
|
You can keep |
henriquemarlon
force-pushed
the
feat/claim-submission-counter
branch
from
c8627c8 to
f33840c
Compare
2 participants
Added getNumberOfSubmittedClaims() to IConsensus interface to expose a monotonic counter of submitted claims, as described in #478
Behavior:
( I’ve also updated the tests ).