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

[SC-380] Improve Coverage + Slight Refactoring #14

Merged
merged 13 commits into from
May 20, 2024

Conversation

hexonaut
Copy link
Contributor

@hexonaut hexonaut commented May 7, 2024

A slight refactoring to the integration test naming, use absolute paths, fix license, add solidity compiler settings and writing unit tests for the business logic.

Please note I am purposefully excluding the min/max delay testing as I'm going to remove it in a subsequent PR since it provides no additional protection.

test/OptimismCrosschainTest.t.sol Outdated Show resolved Hide resolved
test/AuthBridgeExecutor.t.sol Outdated Show resolved Hide resolved
test/AuthBridgeExecutor.t.sol Show resolved Hide resolved
test/AuthBridgeExecutor.t.sol Outdated Show resolved Hide resolved
test/AuthBridgeExecutor.t.sol Show resolved Hide resolved
test/AuthBridgeExecutor.t.sol Outdated Show resolved Hide resolved
test/AuthBridgeExecutor.t.sol Show resolved Hide resolved
test/AuthBridgeExecutor.t.sol Outdated Show resolved Hide resolved
test/AuthBridgeExecutor.t.sol Show resolved Hide resolved
test/AuthBridgeExecutor.t.sol Show resolved Hide resolved
Copy link
Contributor

@barrutko barrutko left a comment

Choose a reason for hiding this comment

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

One minor thing to fix and other than that lgtm

test/AuthBridgeExecutor.t.sol Show resolved Hide resolved
Copy link
Contributor

@lucas-manuel lucas-manuel left a comment

Choose a reason for hiding this comment

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

Small review, still would like to discuss reasoning behind removal of min/max delay validation just so we're 100% certain it won't introduce any issues.

test/AuthBridgeExecutor.t.sol Outdated Show resolved Hide resolved
test/AuthBridgeExecutor.t.sol Outdated Show resolved Hide resolved
@hexonaut hexonaut requested review from barrutko and lucas-manuel May 17, 2024 08:22
Copy link

Coverage after merging SC-380-improve-coverage into master will be

100.00%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
src/executors
   AuthBridgeExecutor.sol100%100%100%100%
   BridgeExecutorBase.sol97.41%89.13%100%100%153, 160, 360, 384–385
src/receivers
   BridgeExecutorReceiverArbitrum.sol100%100%100%100%
   BridgeExecutorReceiverGnosis.sol100%100%100%100%
   BridgeExecutorReceiverOptimism.sol100%100%100%100%

@hexonaut
Copy link
Contributor Author

hexonaut commented May 17, 2024

Fixed all outstanding issues. Re min/max delay: these provide no extra protection because the payload can just call updateMaximumDelay(mySuperLongDelay) before updateDelay(mySuperLongDelay).

@hexonaut hexonaut merged commit 9b9b3af into master May 20, 2024
3 checks passed
@hexonaut hexonaut deleted the SC-380-improve-coverage branch May 20, 2024 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants