Skip to content
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

[NIT-2554][Config Change] Test manual batch-poster fallback for DAS #2665

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

gligneul
Copy link
Contributor

@gligneul gligneul commented Sep 12, 2024

This test checks whether the batch-poster manual fallback for DAS works
correctly. Here are the steps of the test:

  • Setup an L2 chain using a DAS
  • Sends a batch using the DAS
  • Shutdown the DAS
  • Fail to send a batch because the fallback is disabled
  • Enable the fallback
  • Verify the batch was sent with the fallback

Close NIT-2557

Config Change
Rename node.batch-poster.disable-dap-fallback-store-data-on-chain to node.batch-poster.enable-dap-fallback-store-data-on-chain

The batch-poster wasn't using the DAS because the max retention period
wasn't being set correctly.
This test checks whether the batch-poster manual fallback for DAS works
correctly. Here are the steps of the test:

* Setup a L2 chain using a DAS
* Sends a batch using the DAS
* Shutdown the DAS
* Fail to send a batch because the fallback is disabled
* Enable the fallback
* Verify the batch was sent with the fallback
@cla-bot cla-bot bot added the s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA. label Sep 12, 2024
@gligneul gligneul changed the title Test manual batch-poster fallback for DAS [NIT-2554] Test manual batch-poster fallback for DAS Sep 12, 2024
diegoximenes
diegoximenes previously approved these changes Sep 12, 2024
system_tests/das_test.go Outdated Show resolved Hide resolved
ParentChainNodeURL: "none",
RequestTimeout: 5 * time.Second,
}
config := das.DefaultDataAvailabilityConfig
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is something where it would be nice if we could develop an internally-accepted style guide, but:
I really find the inline declaration of the struct more readable. The structure of the data easier to visualize.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, I'm not using a composite literal because I want to initialize all fields based on the default config. The default config had changed, and the DAS tests were not using the correct config because of that.

system_tests/das_test.go Outdated Show resolved Hide resolved
gligneul and others added 2 commits September 16, 2024 10:53
Rename --node.batch-poster.disable-dap-fallback-store-data-on-chain to
--node.batch-poster.enable-dap-fallback-store-data-on-chain
@gligneul gligneul changed the title [NIT-2554] Test manual batch-poster fallback for DAS [NIT-2554][Config Change] Test manual batch-poster fallback for DAS Sep 16, 2024
Copy link
Member

@eljobe eljobe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants