-
Notifications
You must be signed in to change notification settings - Fork 29
chore(rollup): add more tests for rollup bsn #551
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
…s-for-rollup-bsn-fps
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.
✅ Bugbot reviewed your changes and found no bugs!
Bugbot free trial expires on July 31, 2025
Learn more in the Cursor dashboard.
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.
Pull Request Overview
This PR adds comprehensive E2E tests for the rollup BSN (Blockchain Security Network) and updates related infrastructure. The tests mirror the existing Babylon E2E test patterns but are specifically adapted for rollup environments.
- Adds new E2E test suite covering FP lifecycle, double signing protection, and catch-up scenarios
- Removes individual unit-style tests in favor of comprehensive E2E tests
- Updates contract binaries and test configurations for rollup BSN testing
Reviewed Changes
Copilot reviewed 25 out of 29 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| bsn/rollup/e2e/rollup_e2e_test.go | Adds comprehensive E2E tests for rollup BSN finality provider lifecycle |
| bsn/rollup/e2e/rollup_test_manager.go | Updates test manager with Anvil support and contract allowlist management |
| itest/container/container.go | Adds Anvil container support for local Ethereum node testing |
| finality-provider/service/finality_submitter.go | Refactors batch signature submission to handle per-block proofs individually |
| Multiple files | Updates import paths from rollup-finality-provider to rollup directory structure |
Comments suppressed due to low confidence (1)
itest/container/config.go:40
- The Anvil version 'v1.2.3' may not exist or be valid. Foundry/Anvil typically uses versions like 'nightly' or specific commit hashes. Please verify this version exists in the ghcr.io/foundry-rs/foundry repository.
AnvilVersion: "v1.2.3",
| for _, block := range blocks { | ||
| // Get public randomness for this specific height | ||
| pr, err := ds.GetPubRandList(block.GetHeight(), 1) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get public randomness for height %d: %w", block.GetHeight(), err) | ||
| } | ||
| prList = append(prList, pr[0]) | ||
|
|
||
| // Get proof for this specific height | ||
| proofs, err := ds.proofListGetterFunc(block.GetHeight(), 1) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get public randomness inclusion proof for height %d: %w\nplease recover the randomness proof from db", block.GetHeight(), err) | ||
| } | ||
| if len(proofs) != 1 { | ||
| return nil, fmt.Errorf("expected exactly one proof for height %d, got %d", block.GetHeight(), len(proofs)) | ||
| } | ||
| proofBytesList = append(proofBytesList, proofs[0]) | ||
| } | ||
|
|
Copilot
AI
Jul 27, 2025
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 refactored code now makes individual calls to get proofs for each block instead of batch retrieval. This could significantly impact performance when processing many blocks, as it increases the number of database/storage operations from 1 to N.
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.
@gitferry can you take a look at this too?
SebastianElvis
left a comment
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.
Great work 👍 That's quite a lot of things.
Would be great if @Lazar955 can also take a look
Lazar955
left a comment
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.
Lgtm, fix comments I left.
@SebastianElvis @gusin13 Do these additioanl test feel redundant compared to the main FP ones?
I don't think they are redundant. At the end of the day these tests need to exist in every FP. We might consider making the test suite more versatile such that writing e2e tests for BSN FPs becomes easier. We have a Github issue for tracking this #527 |
ah actually yeah you are correct regarding tests |
@Lazar955 I am not sure how to fix this, whenever I add t.Parallel() I keep getting these errors - this needs to be fixed separately - I see such issue is already tracked here |
but this is not due to port conflict error, but due to FP restart, and db file not open |
hm maybe my bad, i thought its due to some port conflicts, i had increased the sleep timer in tests when fp instance restarted which fixed the db open issue thanks for enabling the t.Parallel() again 👍 |
Closes
#521
Partially closes
#528
#535
whats included in this pr
whats not included in this pr/still pending