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.

Sign up for GitHub

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

Jump to bottom

test to ensure DIFFICULTY/PREVRANDAO opcode returns the expected value of 1 #1667

Merged
merged 28 commits into from
Nov 29, 2023
Merged

test to ensure DIFFICULTY/PREVRANDAO opcode returns the expected value of 1 #1667

merged 28 commits into from
Nov 29, 2023

Conversation

amsanghi
Copy link
Contributor

@amsanghi amsanghi commented May 25, 2023 •
edited
Loading

No description provided.

@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 May 25, 2023
@amsanghi amsanghi marked this pull request as ready for review May 25, 2023 17:39
@codecov
Copy link

codecov bot commented May 25, 2023 •
edited
Loading

Codecov Report

Merging #1667 (ceaaa6a) into master (b82a6df) will increase coverage by 0.08%.
The diff coverage is n/a.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1667      +/-   ##
==========================================
+ Coverage   28.87%   28.96%   +0.08%     
==========================================
  Files         222      222              
  Lines       33320    33320              
==========================================
+ Hits         9622     9651      +29     
+ Misses      22620    22583      -37     
- Partials     1078     1086       +8

@amsanghi amsanghi requested a review from tsahee May 30, 2023 18:15
tsahee
tsahee requested changes Jun 13, 2023
View reviewed changes
Copy link
Contributor

@tsahee tsahee left a comment

Choose a reason for hiding this comment

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

I think this should be used in the validator test we discussed

system_tests/common_test.go Outdated Show resolved Hide resolved
system_tests/estimation_test.go Show resolved Hide resolved
tsahee
tsahee requested changes Aug 8, 2023
View reviewed changes
system_tests/common_test.go Outdated Show resolved Hide resolved
system_tests/estimation_test.go Outdated Show resolved Hide resolved
system_tests/block_validator_test.go Show resolved Hide resolved
@amsanghi amsanghi mentioned this pull request Oct 25, 2023
Merged
@amsanghi amsanghi requested a review from tsahee October 25, 2023 12:06
tsahee
tsahee previously approved these changes Oct 25, 2023
View reviewed changes
Copy link
Contributor

@tsahee tsahee left a comment

Choose a reason for hiding this comment

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

the change LGTM - however, if you have can - this is a great opportunity to try converting TestDifficulty* and testBlockValidatorSimple to use the new builder in common_test. Using this framework should avoid the need to modify CreateTestL2WithConfig and the other tests accordingly.

@amsanghi
Copy link
Contributor Author

the change LGTM - however, if you have can - this is a great opportunity to try converting TestDifficulty* and testBlockValidatorSimple to use the new builder in common_test. Using this framework should avoid the need to modify CreateTestL2WithConfig and the other tests accordingly.

so i tried the builder pattern and it worked great with TestDifficulty* (updated the PR with the latest changes) but testBlockValidatorSimple it failing to work with following error "a data availability service is required for this chain, but it was not configured" I guess the builder does not support DAS (nova) right now?

@amsanghi
Copy link
Contributor Author

the change LGTM - however, if you have can - this is a great opportunity to try converting TestDifficulty* and testBlockValidatorSimple to use the new builder in common_test. Using this framework should avoid the need to modify CreateTestL2WithConfig and the other tests accordingly.

so i tried the builder pattern and it worked great with TestDifficulty* (updated the PR with the latest changes) but testBlockValidatorSimple it failing to work with following error "a data availability service is required for this chain, but it was not configured" I guess the builder does not support DAS (nova) right now?

Resolved now!

@joshuacolvin0
Copy link
Member

The nitro-contracts develop branch needs to be merged into OffchainLabs/nitro-contracts#84 before this PR can be merged.

joshuacolvin0
joshuacolvin0 previously approved these changes Nov 21, 2023
View reviewed changes
Copy link
Member

@joshuacolvin0 joshuacolvin0 left a comment

Choose a reason for hiding this comment

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

LGTM

@amsanghi amsanghi enabled auto-merge November 28, 2023 17:30
joshuacolvin0
joshuacolvin0 approved these changes Nov 28, 2023
View reviewed changes
Copy link
Member

@joshuacolvin0 joshuacolvin0 left a comment

Choose a reason for hiding this comment

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

LGTM

@amsanghi amsanghi merged commit 7699979 into master Nov 29, 2023
9 checks passed
@amsanghi amsanghi deleted the test_PREVRANDAO branch November 29, 2023 02:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PlasmaPower PlasmaPower PlasmaPower left review comments

@joshuacolvin0 joshuacolvin0 joshuacolvin0 approved these changes

@tsahee tsahee tsahee left review comments

Assignees
No one assigned
Labels
s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@amsanghi @joshuacolvin0 @PlasmaPower @tsahee