Feat: fast bridge refunds #2219
Conversation
WalkthroughThe updates across the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2219 +/- ##
===================================================
+ Coverage 47.84420% 47.89034% +0.04614%
===================================================
Files 360 366 +6
Lines 26881 27066 +185
Branches 83 132 +49
===================================================
+ Hits 12861 12962 +101
- Misses 12680 12763 +83
- Partials 1340 1341 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (6)
- packages/contracts-rfq/contracts/Admin.sol (3 hunks)
- packages/contracts-rfq/contracts/FastBridge.sol (6 hunks)
- packages/contracts-rfq/contracts/interfaces/IAdmin.sol (1 hunks)
- packages/contracts-rfq/foundry.toml (1 hunks)
- packages/contracts-rfq/script/FastBridge.s.sol (1 hunks)
- packages/contracts-rfq/test/FastBridge.t.sol (37 hunks)
Additional comments: 20
packages/contracts-rfq/foundry.toml (1)
- 4-6: The addition of
block_timestampandevm_versionsettings in thefoundry.tomlfile is a positive change, enhancing the testing environment by specifying a fixed block timestamp and EVM version for contract testing. This ensures consistency and predictability in test outcomes, which is crucial for contract development and testing.packages/contracts-rfq/contracts/interfaces/IAdmin.sol (1)
- 4-16: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-14]
The modifications in
IAdmin.sol, focusing on simplifying the interface by removing outdated events and functions related to relayers, guards, and governors, are well-aligned with the PR objectives. This change streamlines the interface, focusing on fee rate updates, fee sweeping, and chain gas amount updates, which are more critical to the contract's operation.packages/contracts-rfq/script/FastBridge.s.sol (1)
- 20-20: The modification to use
grantRolefor adding relayers in theDeployFastBridgescript aligns with the PR's objective of refining role-based access control. This approach enhances flexibility and control over role assignments, ensuring that role management is more adaptable and easier to maintain.packages/contracts-rfq/contracts/Admin.sol (2)
- 13-13: The introduction of the
REFUNDER_ROLEconstant in theAdmincontract is a key update that aligns with the PR's objective of enhancing role-based access control, specifically for processing refunds. This addition is crucial for delineating permissions in a more structured manner.- 33-33: Replacing specific role modifiers with the generic
onlyRolemodifier in functions such assetProtocolFeeRate,sweepProtocolFees, andsetChainGasAmountis a significant improvement. This change streamlines the enforcement of role-based access control, enhancing security and flexibility by allowing for more granular control over permissions.Also applies to: 40-40, 49-49
packages/contracts-rfq/contracts/FastBridge.sol (2)
- 19-20: Replacing
PROVE_PERIODwithREFUND_DELAYin theFastBridgecontract is a significant update that aligns with the PR's objective of refining the refund mechanism. This change adjusts the eligibility criteria for refunds, aiming to improve the contract's functionality and security around the refund process.- 127-127: Implementing role-based access control for critical functions such as
relay,prove,claim,dispute, andrefundin theFastBridgecontract is a key enhancement. This approach ensures that only authorized roles can execute these functions, significantly improving the contract's security and operational integrity. The logic changes in theproveandrefundfunctions for deadline checks are also well-implemented, further enhancing the contract's efficiency and security.Also applies to: 172-172, 203-203, 228-228, 244-250
packages/contracts-rfq/test/FastBridge.t.sol (13)
- 14-14: Importing
IAccessControlfrom OpenZeppelin is a good practice for role management. Ensure that the OpenZeppelin contracts version is compatible with the Solidity compiler version (^0.8.13) used in this project.- 19-19: The
TX_DEADLINEconstant is introduced to manage the deadline for transactions. It's crucial to ensure that this value is appropriately chosen based on the expected transaction finality time and network conditions.- 27-27: Adding a
refunderaddress for testing the refund mechanism is a good practice. Ensure that this address is used consistently in tests related to refunds.- 63-63: The use of
TX_DEADLINEfor calculating deadlines in various bridge transaction tests is consistent and aligns with the changes mentioned in the PR objectives. This ensures that the tests reflect the updated contract logic.Also applies to: 102-102, 141-141, 180-180, 458-458, 497-497, 558-558, 620-620, 681-681, 730-730, 781-781, 813-813, 838-838, 863-863, 888-888, 913-913
- 206-208: The
expectUnauthorizedfunction is a useful addition for testing role-based access control. It simplifies the process of expecting revert messages for unauthorized access attempts.- 212-215: Granting roles using the
grantRolefunction in thesetUpRolesmethod aligns with the PR objectives to streamline role management. Ensure that all necessary roles are granted for the tests to run correctly.- 232-232: Replacing specific role management functions with
grantRoleandrevokeRolein tests is consistent with the PR objectives. This change enhances flexibility and control over role assignments and revocations.Also applies to: 240-242, 250-250, 259-261, 268-268, 276-278, 286-286, 295-297, 304-304, 311-313, 320-320, 328-330, 334-337, 341-345, 349-353, 357-362
- 1299-1299: Using the
expectUnauthorizedutility function to test role-based access control in thetest_failedRelayNotRelayerfunction demonstrates good testing practices and consistency.- 1388-1404: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1375-1401]
Renaming
test_failedProveTimeExceededtotest_proveWithHugeDelayand adjusting its logic to test the prove function with a significant delay is a meaningful update. It's important to ensure that the test accurately reflects the contract's behavior under such conditions.
- 1773-1773: The
expectUnauthorizedfunction is effectively used again intest_failedDisputeNotGuardto test access control for the dispute function, showcasing consistent testing methodology.- 1786-1786: The introduction of refund-related tests, including permissionless refunds after a certain delay, aligns with the PR objectives to implement a refined refund mechanism. Ensure that these tests cover all scenarios, including edge cases, to fully validate the refund logic.
Also applies to: 1819-1819, 1847-1847, 1881-1881, 1908-1908, 1934-1934, 1953-1953, 1960-1960, 1987-1987, 2009-2009
- 1943-1947: The
test_failedRefundNotEnoughTimefunction correctly tests the refund mechanism's timing constraints. It's crucial to ensure that the timing logic in the contract aligns with these tests to prevent premature refunds.- 1983-1984: The
test_successfulRefundNotRefunderfunction demonstrates the flexibility of the refund mechanism, allowing permissionless refunds after a specific period. This test is essential for verifying that the contract behaves as expected in permissionless refund scenarios.
github-actions
bot
removed
needs-go-generate-agents
needs-go-generate-services/rfq
labels
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (1)
- packages/contracts-rfq/contracts/Admin.sol (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/contracts-rfq/contracts/Admin.sol
* Add `solidity-devops` package * Add devops configs * Chore: consistent formatting across packages * Rm broadcast folder * Add broadcast to gitignore * Add deployment script * Add configuration script * Add bash scripts * Bump solidity-devops version
Deploying with
|
| Latest commit: |
cb1fe6f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f56ef281.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-fast-bridge-refunds.sanguine-fe.pages.dev |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (15)
- package.json (1 hunks)
- packages/contracts-rfq/.env.example (1 hunks)
- packages/contracts-rfq/.gitignore (1 hunks)
- packages/contracts-rfq/configs/global/FastBridge.json (1 hunks)
- packages/contracts-rfq/devops.json (1 hunks)
- packages/contracts-rfq/foundry.toml (1 hunks)
- packages/contracts-rfq/package.json (1 hunks)
- packages/contracts-rfq/remappings.txt (1 hunks)
- packages/contracts-rfq/script/ConfigureFastBridge.s.sol (1 hunks)
- packages/contracts-rfq/script/DeployFastBridge.CREATE2.s.sol (1 hunks)
- packages/contracts-rfq/script/fb-config.sh (1 hunks)
- packages/contracts-rfq/script/fb-deploy.sh (1 hunks)
- packages/contracts-rfq/test/FastBridge.t.sol (52 hunks)
- packages/contracts-rfq/test/FastBridgeMock.sol (1 hunks)
- packages/contracts-rfq/test/UniversalTokenLib.t.sol (8 hunks)
Files skipped from review due to trivial changes (3)
- packages/contracts-rfq/devops.json
- packages/contracts-rfq/package.json
- packages/contracts-rfq/test/FastBridgeMock.sol
Files skipped from review as they are similar to previous changes (1)
- packages/contracts-rfq/foundry.toml
Additional comments: 20
packages/contracts-rfq/configs/global/FastBridge.json (1)
- 1-10: The configuration file
FastBridge.jsoncorrectly defines the new roles and settings as outlined in the PR objectives. It's important to ensure that these configurations are correctly utilized in the contract implementations and tests.packages/contracts-rfq/remappings.txt (1)
- 2-4: The added package path mappings for
@synapsecns/solidity-devops,forge-std, andds-testare correctly formatted and align with the PR objectives to enhance the development and testing environment. Ensure that these dependencies are properly utilized in the contracts and scripts.packages/contracts-rfq/script/fb-config.sh (1)
- 1-15: The
fb-config.shscript is well-structured and follows good practices for bash scripting, including proper usage of variables and conditional checks. Ensure that theConfigureFastBridge.s.solscript referenced is correctly implemented to handle these configurations.packages/contracts-rfq/script/fb-deploy.sh (1)
- 1-15: The
fb-deploy.shscript follows the same structure and best practices as thefb-config.shscript. It's important to verify that theDeployFastBridge.CREATE2.s.solscript referenced correctly implements the deployment logic for the FastBridge contract.packages/contracts-rfq/script/DeployFastBridge.CREATE2.s.sol (2)
- 17-24: Ensure that the
deployAndSavefunction correctly deploys theFastBridgecontract using CREATE2 and saves the deployment address. This is crucial for deterministic deployment and should be thoroughly tested.- 26-31: The
afterExecutionfunction provides a good example of post-deployment checks. It's important to ensure that these checks (e.g.,checkAdminCountandcheckAdmin) are comprehensive and align with the contract's expected state after deployment.packages/contracts-rfq/.env.example (1)
- 1-47: The
.env.examplefile is comprehensive and provides clear instructions for configuring wallets and chains. It's crucial to remind users to never commit their.envfile with sensitive information (e.g., private keys) to version control.package.json (1)
- 15-15: Adding
@synapsecns/solidity-devopsto thenohoistsection inpackage.jsonis appropriate for ensuring that this dependency is correctly managed in the workspace. This aligns with the PR's objective to enhance the development and testing environment.packages/contracts-rfq/script/ConfigureFastBridge.s.sol (1)
- 16-23: The
runfunction inConfigureFastBridge.s.solcorrectly implements the logic to configure theFastBridgecontract based on the loaded configuration. Ensure that the role syncing functions (syncRole) correctly manage role assignments and revocations as intended.packages/contracts-rfq/test/UniversalTokenLib.t.sol (1)
- 23-23: Using underscores in numeric literals (e.g.,
12_345) for theamountvariable improves readability and is a good practice. Ensure that all tests correctly validate the functionality they are intended to test.packages/contracts-rfq/test/FastBridge.t.sol (10)
- 14-14: Importing
IAccessControlfrom OpenZeppelin is a good practice for role management. Ensure that the version of OpenZeppelin contracts is compatible with the Solidity compiler version (^0.8.13) used in this project.- 19-19: The
TX_DEADLINEconstant is introduced to define the deadline for bridge transactions. It's set to 60 minutes, which seems reasonable for most use cases. However, consider if this value should be configurable to adapt to different operational requirements or network conditions.- 27-27: Adding a
refunderaddress is aligned with the PR's objective to enhance role-based access control, specifically for processing refunds. Ensure that therefunderrole is properly managed and secured, as it has critical permissions.- 222-224: The
expectUnauthorizedfunction is a useful addition for testing unauthorized access scenarios. It leverages thevm.expectRevertfunction from Foundry to simulate and assert the expected revert behavior when unauthorized roles attempt to perform restricted actions.- 228-235: Granting roles using
fastBridge.grantRoleforRELAYER_ROLE,GUARD_ROLE,GOVERNOR_ROLE, andREFUNDER_ROLEand verifying them withassertTrueis correctly implemented. This setup ensures that the roles are correctly assigned before running the tests. Good use of Foundry'svm.startPrankandvm.stopPrankto simulate transactions from specific addresses.- 474-474: The use of
TX_DEADLINEto calculate thedeadlineparameter for the bridge transaction is consistent with the introduction of theTX_DEADLINEconstant. This ensures that the bridge transaction must be processed within the specified deadline, enhancing the security and predictability of bridge operations.- 1404-1420: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1391-1417]
The
test_proveWithHugeDelayfunction simulates a scenario where the relay proof is provided after a significant delay (30 days). This test is important for ensuring that the system behaves correctly even when proofs are delayed. However, it's crucial to ensure that the delay duration aligns with the intended dispute resolution and refund policies of the FastBridge contract.
- 1802-1804: The
test_successfulRefundfunction correctly simulates the refund process by skipping time beyond theTX_DEADLINEand then calling therefundfunction. This test is crucial for ensuring that refunds are processed correctly when the conditions are met. It's important to ensure that the refund logic securely validates the conditions under which a refund is permissible.- 1950-1967: The
test_failedRefundNotEnoughTimefunction correctly tests the scenario where a refund attempt is made before theTX_DEADLINEhas passed. This test ensures that the contract correctly enforces the deadline before allowing refunds. It's a good practice to cover such negative test cases to ensure robustness.- 2009-2016: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [2003-2013]
The
test_failedRefundNotRefunderNotEnoughTimefunction tests the scenario where a non-refunder attempts a permissionless refund before the allowed period. This test is important for ensuring that the contract enforces both role-based and time-based restrictions on refunds. Consider clarifying in the test name or comments that this tests the permissionless refund mechanism's timing restrictions.
Description
A clear and concise description of the features you're adding in this pull request.
Additional context
Add any other context about the problem you're solving.
Metadata
Summary by CodeRabbit
REFUNDER_ROLEand generic role management functions in the Admin contract.REFUND_DELAYparameter and revamped access control for key functions.onlyRolemodifier for better code maintenance.foundry.tomlfor consistent development practices.