-
Notifications
You must be signed in to change notification settings - Fork 3.6k
use opcm.addGameType in deploy script #15575
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #15575 +/- ##
===========================================
- Coverage 46.04% 45.94% -0.11%
===========================================
Files 1279 1279
Lines 105475 105475
===========================================
- Hits 48564 48456 -108
- Misses 53413 53530 +117
+ Partials 3498 3489 -9
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
a90e5d9
to
62e8ca8
Compare
… into amadi/use-addgametype-in-deploy
packages/contracts-bedrock/test/invariants/OptimismPortal2.t.sol
Outdated
Show resolved
Hide resolved
packages/contracts-bedrock/test/periphery/monitoring/DisputeMonitorHelper.t.sol
Show resolved
Hide resolved
This LGTM once @clabby has looked at the changes to the invariant tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The invariant test you had to modify is pretty poor, sorry about that. The only reason it's currently passing is because the foundry fuzzer has a near-zero chance of actually selecting a bond that's suitable to create a new move, meaning the actor is being called by the fuzzer, but it never actually lands any moves in the game. If it did, this test would fail, and you can see that it's currently reverting on n/n
calls in the logs.
I'd recommend modifying the actor to be more smart about the bond that it picks, and remove that as a fuzzed value. The diff I've left below does that, and you can validate that the current assertions are incorrect by only applying the changes to the actor.
Good news is everything is behaving as expected, this test has just been dead weight since we added the restrictions for exact bond amounts in #9924.
diff --git a/packages/contracts-bedrock/lib/forge-std b/packages/contracts-bedrock/lib/forge-std
index 6853b9ec7..6abf66980 160000
--- a/packages/contracts-bedrock/lib/forge-std
+++ b/packages/contracts-bedrock/lib/forge-std
@@ -1 +1 @@
-Subproject commit 6853b9ec7df5dc0c213b05ae67785ad4f4baa0ea
+Subproject commit 6abf66980050ab03a35b52bdab814f55001d6929
diff --git a/packages/contracts-bedrock/test/invariants/FaultDisputeGame.t.sol b/packages/contracts-bedrock/test/invariants/FaultDisputeGame.t.sol
index d30ab10b4..70dc7d4ea 100644
--- a/packages/contracts-bedrock/test/invariants/FaultDisputeGame.t.sol
+++ b/packages/contracts-bedrock/test/invariants/FaultDisputeGame.t.sol
@@ -37,6 +37,14 @@ contract FaultDisputeGame_Solvency_Invariant is FaultDisputeGame_Init {
function invariant_faultDisputeGame_solvency() public {
vm.warp(block.timestamp + 7 days + 1 seconds);
+ (,,, uint256 rootBond,,,) = gameProxy.claimData(0);
+
+ // Ensure the root bond is greater than 0.
+ assertGt(address(this).balance, 0);
+
+ // Ensure the game creator has locked up the root bond.
+ assertEq(address(this).balance, type(uint96).max - rootBond);
+
for (uint256 i = gameProxy.claimDataLen(); i > 0; i--) {
(bool success,) = address(gameProxy).call(abi.encodeCall(gameProxy.resolveClaim, (i - 1, 0)));
assertTrue(success);
@@ -71,14 +79,21 @@ contract FaultDisputeGame_Solvency_Invariant is FaultDisputeGame_Init {
}
if (gameProxy.status() == GameStatus.DEFENDER_WINS) {
+ // In the event that the defender wins, they receive their bond back. The root claim is never paid out
+ // bonds from claims below it, so the actor that has challenged the root claim (and potentially their)
+ // own receives all of their bonds back.
assertEq(address(this).balance, type(uint96).max);
+ assertEq(address(actor).balance, actor.totalBonded());
} else if (gameProxy.status() == GameStatus.CHALLENGER_WINS) {
- assertEq(DEFAULT_SENDER.balance, type(uint96).max);
+ // If the defender wins, the game creator loses the root bond and the actor receives it. The actor also
+ // is the only party that may have challenged their own claims, so we expect them to receive all of them
+ // back.
+ assertEq(address(this).balance, type(uint96).max - rootBond);
+ assertEq(address(actor).balance, actor.totalBonded() + rootBond);
} else {
revert("FaultDisputeGame_Solvency_Invariant: unreachable");
}
- assertEq(address(actor).balance, actor.totalBonded());
assertEq(address(gameProxy).balance, 0);
}
}
@@ -94,14 +109,18 @@ contract RandomClaimActor is StdUtils {
VM = _vm;
}
- function move(bool _isAttack, uint256 _parentIndex, Claim _claim, uint64 _bondAmount) public {
- _parentIndex = bound(_parentIndex, 0, GAME.claimDataLen());
- VM.deal(address(this), _bondAmount);
+ function move(bool _isAttack, uint256 _parentIndex, Claim _claim) public {
+ _parentIndex = bound(_parentIndex, 0, GAME.claimDataLen() - 1);
+
+ (,,,,,Position parentPos,) = GAME.claimData(_parentIndex);
+ Position nextPosition = parentPos.move(_isAttack);
+ uint256 bondAmount = GAME.getRequiredBond(nextPosition);
- totalBonded += _bondAmount;
+ VM.deal(address(this), bondAmount);
+ totalBonded += bondAmount;
(,,,, Claim disputed,,) = GAME.claimData(_parentIndex);
- GAME.move{ value: _bondAmount }(disputed, _parentIndex, _claim, _isAttack);
+ GAME.move{ value: bondAmount }(disputed, _parentIndex, _claim, _isAttack);
}
fallback() external payable { }
thanks @clabby i've pushed the changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
use opcm.addGameType in
setCannonFaultGameImplementation
deploy script function