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

Test utils #106

Merged
merged 22 commits into from
Dec 9, 2022
Merged

Test utils #106

merged 22 commits into from
Dec 9, 2022

Conversation

gh-23378
Copy link
Contributor

@gh-23378 gh-23378 commented Dec 4, 2022

No description provided.

revert LibErrors.NewResurrectionTimeInPast(resurrectionTime);
}

// Confirm that new resurrection time doesn't exceed sarcophagus's maximumRewrapInterval
if (resurrectionTime > block.timestamp + sarcophagus.maximumRewrapInterval) {
if (block.timestamp + sarcophagus.maximumRewrapInterval < resurrectionTime) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason you seem to be making sure block.timestamp appears earlier in these expressions? Some efficiency/edgecase shenanigans maybe? Or just a style choice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is just a style thing haha I was running through all the code trying to evaluate it for readability and I personally think it's easier to follow if block timestamp is consistently the first thing in the checks

@@ -39,6 +39,7 @@ struct AppStorage {
mapping(bytes32 => address) doubleHashedShardArchaeologists;

// sarcophagus ids
// todo: is this used?
Copy link
Contributor

Choose a reason for hiding this comment

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

I highly doubt it. Seth and Kyle may have more context on if there was some future plans for having this

@gh-23378 gh-23378 merged commit e37b053 into main Dec 9, 2022
@sethhrbek sethhrbek deleted the test-utils branch March 30, 2023 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants