Add e2e test case to check for monomer rollbacks #192
Conversation
WalkthroughThe pull request introduces enhancements to the Monomer blockchain's end-to-end testing framework. A new test case, "No Rollbacks," is added to monitor block height and detect any rollbacks during execution. Additionally, modifications to the Changes
Possibly related issues
Possibly related PRs
Poem
Tip New featuresWalkthrough comment now includes:
Notes:
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
e2e/stack_test.go
Outdated
| require.Fail(t, "monomer has rolled back") | ||
| } | ||
| // End the test once the target monomer block height is reached | ||
| if currentBlock.Number().Int64() >= int64(15) { |
There was a problem hiding this comment.
question for reviewers - With a SWS of 4, L1 block time of 2 ,and L2 block time of 1, the rollbacks will always occur at monomer block 13 so this check is sufficient. However, if any config param ever changes, the target monomer block height may need to be tuned to correctly check for rollbacks. Should I make calculating the target block height more formulaic based on those config params?
There was a problem hiding this comment.
We should hit the rollback after a single sequencing window has passed on L1, correct? Therefore, it makes sense to wait one SWS + 1 L1 blocks.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- e2e/stack_test.go (3 hunks)
Additional comments not posted (3)
e2e/stack_test.go (3)
12-12: LGTM!The
timepackage import is approved.
60-63: LGTM!The "No Rollbacks" test case declaration is approved.
125-143: LGTM!The
checkForRollbacksfunction is approved. It correctly checks for rollbacks by comparing the current block height with the last checked block height and triggers a failure if a rollback is detected. The function also terminates the test once the target block height is reached and introduces a delay between checks to avoid excessive querying.
Checks to ensure that the latest monomer block height is never less than the last checked monomer block height.
natebeauregard
force-pushed
the
nkb.add-rollback-e2e-test
branch
from
c1da0c5 to
dd88c16
Compare
|
force-pushed to rebase on main |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- e2e/stack.go (3 hunks)
- e2e/stack_test.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- e2e/stack_test.go
Additional comments not posted (3)
e2e/stack.go (3)
56-56: Field addition approved:SequencerWindowSize.The addition of the
SequencerWindowSizefield to theStackConfigstruct is appropriate and well-typed asuint64. This change enhances the configuration capabilities of the stack, aligning with the PR's objectives to manage transaction sequencing more effectively.
254-254: Verify the impact of hardcoded endpoint in L2 client instantiation.The change to use a hardcoded string
"/websocket"as the endpoint in thebftclient.Newfunction call simplifies the connection setup but may reduce flexibility. It's important to verify that this change does not adversely affect the communication protocol or endpoint used in different environments.Consider making the endpoint configurable to maintain flexibility across different deployment scenarios.
259-264: Proper management of L2 client lifecycle.The addition of the
l2Client.Start()method call and its subsequent error handling, along with the deferred stopping of the Comet client, are crucial for ensuring the L2 client is properly managed throughout its lifecycle. These changes enhance the robustness and reliability of the client management within therunmethod.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- e2e/stack.go (4 hunks)
- e2e/stack_test.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- e2e/stack_test.go
Additional comments not posted (4)
e2e/stack.go (4)
24-24: LGTM!The import statement for the
rolluppackage is valid and follows the correct syntax.
55-55: LGTM!The new field
RollupConfigof type*rollup.Confighas been correctly added to theStackConfigstruct. This field enhances the configuration capabilities of the stack by allowing it to specify rollup-related settings.
255-255: Verify the change to the L2 client connection.The second argument of the
bftclient.Newfunction call has been changed froms.monomerCometURL.String()to a hardcoded string"/websocket". This change may indicate a shift in how the L2 client connects to the Comet service, potentially affecting the communication protocol or endpoint used.Please ensure that this change is intentional and does not introduce any issues with the L2 client connection. Provide additional context or documentation if necessary.
260-265: LGTM!The code block for starting and stopping the L2 client is well-implemented and follows best practices:
- The L2 client is actively started using
l2Client.Start(), ensuring it is properly initialized within the lifecycle of therunmethod.- Error handling is performed for the
l2Client.Start()operation, adhering to proper error management.- The deferred stopping of the client using
env.DeferErrensures proper cleanup and resource management.The code block enhances the control flow and improves the overall reliability of the L2 client management.
Checks to ensure that the latest monomer block height is never less than the last checked monomer block height.
Rollbacks can be triggered to make the test fail by commenting out this line in stack.go.
Summary by CodeRabbit
New Features
Bug Fixes