Feat: synapse module cleanup

[ChiTimesChi]

Description

• Added the ability to add and remove multiple verifiers at the same time.
• General cleanup.
• Gas limit for verifyEntry() is not configurable, with a default value of 100k gas units.
  • This makes the verification fee setting a bit easier.
• Changed the fee collection workflow:
  • On every verification request the verification fees are accumulated by SynapseModule
  • Anyone could call claimFees() to transfer the accumulated fees to feeCollector (assuming it's been set)
  • There is a configurable claim fee paid to the caller of claimFees() function to incentivize claiming by the third parties.

Summary by CodeRabbit

• New Features
  • Introduced new events for gas limit verification and claim fee fraction changes.
  • Added functionality for adding and removing multiple verifiers.
  • Implemented new settings for claim fee fraction and gas limits.
• Enhancements
  • Refined fee claiming, entry verification, and transaction cost estimation processes.
• Refactor
  • Significant updates to the SynapseModule contract for improved management of verifiers and fees.
  • Removed the decodeEntry function for efficiency.
• Tests
  • Updated and added tests for verifier management, fee setting, and gas limit adjustments.

[coderabbitai]

Important

Auto Review Skipped

Auto reviews are disabled on base/target branches other than the default branch. Please add the base/target branch pattern to the list of additional branches to be reviewed in the settings.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository.

To trigger a single review, invoke the @coderabbitai review command.

Walkthrough

The recent updates focus on enhancing the SynapseModule and related contracts within the contracts-communication package. New events, error types, and functions have been introduced to improve verifier management, fee collection, and gas estimation. Significant changes include the ability to add/remove multiple verifiers, adjust claim fee fractions and gas limits, and refine logic for fee claiming and transaction cost estimation. These modifications aim to optimize the module's functionality and structure for better performance and flexibility.

Changes

Files	Change Summary
.../events/SynapseModuleEvents.sol	Added VerifyGasLimitChanged, ClaimFeeFractionChanged events.
.../interfaces/ISynapseModule.sol	New error types, functions for verifiers, fee settings, and gas limits.
.../modules/InterchainModule.sol	Removed decodeEntry function, updated _requestVerification, minor restructure.
.../modules/SynapseModule.sol	Major updates: default gas limit, claim fee fraction/gas limits variables, new verifier/fee functions, refined logic.
.../test/modules/SynapseModule.Destination.t.sol	Adjusted imports, disabled solhint directives, removed InterchainEntry import.
.../test/modules/SynapseModule.Management.t.sol, .../Source.t.sol	New tests for verifiers, fee fractions, gas limits; renamed and refactored functions, adjusted event and logic handling.

🐰✨
Changes abound, the code does leap,
Through lines and functions, the updates seep.
Verifiers gather, fees take their stand,
In the realm of code, at our command.
With every tweak and every fix,
We craft the future, with our tricks.
🌟🐇

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?

Share

• X
• Mastodon
• Reddit
• LinkedIn

---

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>.
  • Generate unit-tests for this file.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit tests for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai generate interesting stats about this repository from git and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @coderabbitai read src/utils.ts and generate unit tests.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

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)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• The JSON schema for the configuration file is available here.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

[ChiTimesChi]

@coderabbitai review

[coderabbitai]

@ChiTimesChi: I will review the PR.

Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 46.57023%. Comparing base (49772bb) to head (1ca6907).
Report is 15 commits behind head on feat/communication.

Additional details and impacted files

@@                     Coverage Diff                      @@
##           feat/communication       #2118         +/-   ##
============================================================
- Coverage            46.57615%   46.57023%   -0.00592%     
============================================================
  Files                     230         230                 
  Lines                   16867       16867                 
  Branches                   83          83                 
============================================================
- Hits                     7856        7855          -1     
  Misses                   8170        8170                 
- Partials                  841         842          +1     

Flag	Coverage Δ
packages	90.51808% <ø> (-0.09777%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: .coderabbit.yaml

Commits

Files that changed from the base of the PR and between dbb8b66 and f3e196e.

Files selected for processing (7)

• packages/contracts-communication/contracts/events/SynapseModuleEvents.sol (1 hunks)
• packages/contracts-communication/contracts/interfaces/ISynapseModule.sol (5 hunks)
• packages/contracts-communication/contracts/modules/InterchainModule.sol (2 hunks)
• packages/contracts-communication/contracts/modules/SynapseModule.sol (6 hunks)
• packages/contracts-communication/test/modules/SynapseModule.Destination.t.sol (1 hunks)
• packages/contracts-communication/test/modules/SynapseModule.Management.t.sol (5 hunks)
• packages/contracts-communication/test/modules/SynapseModule.Source.t.sol (4 hunks)

Additional comments: 42

packages/contracts-communication/contracts/events/SynapseModuleEvents.sol (2)

• 11-11: The VerifyGasLimitChanged event is well-defined and aligns with the PR's objective to introduce a configurable gas limit for the verifyEntry() function. It correctly logs the chain ID and the new gas limit.
• 13-13: The ClaimFeeFractionChanged event is correctly defined to log changes in the claim fee fraction. This supports the PR's goal of overhauling the fee collection workflow and incentivizing the claiming process.

packages/contracts-communication/contracts/modules/InterchainModule.sol (1)

• 60-61: The addition of an empty _requestVerification function (marked with solhint-disable-next-line no-empty-blocks) is a placeholder for future implementations or overrides in derived contracts. While it's currently empty, ensure that derived contracts properly implement this function as needed.

packages/contracts-communication/contracts/interfaces/ISynapseModule.sol (6)

• 7-10: The new error types introduced (SynapseModule__ClaimFeeFractionExceedsMax, SynapseModule__FeeCollectorNotSet, SynapseModule__GasOracleNotContract, SynapseModule__NoFeesToClaim) are well-defined and provide clear error messages for specific failure conditions. This enhances the contract's readability and error handling.
• 19-22: The addVerifiers function allows for adding multiple verifiers in a single transaction, improving efficiency. Ensure that the implementation checks for duplicates and already added verifiers to prevent unnecessary state changes or gas wastage.
• 29-32: Similarly, the removeVerifiers function streamlines the process of removing multiple verifiers. It's important to ensure that the implementation handles cases where one or more verifiers are not currently added, to avoid reverting the entire transaction unnecessarily.
• 44-48: The setClaimFeeFraction function introduces a mechanism to set the fraction of fees claimable by the caller of claimFees. It's crucial to validate that the claim fee fraction does not exceed the maximum allowed value (MAX_CLAIM_FEE_FRACTION) to maintain system integrity and prevent excessive claiming.
• 55-59: The setVerifyGasLimit function allows for setting a custom gas limit for verifying entries on specific chains. This is a significant feature that adds flexibility to the gas limit configuration. Ensure that the gas limit values are validated to prevent setting excessively high or low limits that could impact the contract's functionality or economics.
• 63-68: The claimFees function is a key part of the fee collection workflow overhaul. It should correctly calculate the claimer's fee based on the _claimFeeFraction and transfer the remaining fees to the feeCollector. Proper error handling for unset feeCollector and no fees to claim conditions is essential.

packages/contracts-communication/contracts/modules/SynapseModule.sol (10)

• 17-18: The DEFAULT_VERIFY_GAS_LIMIT constant is set to a reasonable default of 100,000 gas units. This aligns with the PR's objective to introduce a configurable gas limit for the verifyEntry() function. Ensure this default value is appropriate for the expected operations within verifyEntry.
• 20-28: The introduction of MAX_CLAIM_FEE_FRACTION and FEE_PRECISION constants, alongside the _claimFeeFraction and _verifyGasLimit internal variables, is well-structured. These changes support the new functionalities for managing claim fee fractions and gas limits. Ensure that the use of these variables adheres to the intended logic, especially in calculations involving percentages and gas limits.
• 43-51: The implementation of addVerifiers using a loop to add each verifier individually is efficient. However, ensure that the _addVerifier internal function properly handles cases where a verifier is already added to prevent unnecessary state changes.
• 56-64: Similarly, the removeVerifiers function's loop implementation for removing verifiers is straightforward. It's important to ensure that the _removeVerifier internal function gracefully handles cases where a verifier is not currently added, to avoid transaction failures.
• 79-85: The setClaimFeeFraction function correctly checks if the provided claimFeeFraction exceeds the MAX_CLAIM_FEE_FRACTION. This validation is crucial for maintaining the integrity of the fee claiming process. The emission of the ClaimFeeFractionChanged event upon successful update is a good practice for transparency.
• 97-101: The setVerifyGasLimit function allows for flexible gas limit configuration per chain ID. The use of a mapping to store gas limits for different chain IDs is appropriate. Ensure that the gas limits set are validated against reasonable bounds to prevent misuse.
• 105-118: The claimFees function's implementation, including error checks for an unset feeCollector and no fees to claim, is in line with the PR's objectives. The calculation of the claimer's fee and the distribution of fees are crucial aspects. Ensure that the fee calculation and transfers are secure and accurate.
• 129-132: The getClaimFeeFraction function provides a simple and direct way to retrieve the current claim fee fraction. This supports the enhanced fee collection workflow by allowing external entities to query the claim fee fraction.
• 144-147: The getClaimFeeAmount function calculates the claim fee amount based on the contract's balance and the claim fee fraction. Ensure that the calculation is secure against rounding errors and that the division by FEE_PRECISION is handled correctly.
• 154-159: The getVerifyGasLimit function correctly returns a custom gas limit if set, or the default gas limit otherwise. This implementation supports the configurable gas limit feature. Validate that the default gas limit is used appropriately when no custom limit is set.

packages/contracts-communication/test/modules/SynapseModule.Source.t.sol (14)

• 7-8: The reordering of imports and the addition of necessary contracts and libraries for testing are appropriate. This ensures that all dependencies are correctly included for comprehensive testing of the SynapseModule.
• 23-23: The addition of the claimer address variable is a good practice for testing fee claiming functionalities. This allows for simulating different actors interacting with the contract in a testing environment.
• 28-28: Renaming EXPECTED_GAS_LIMIT to DEFAULT_GAS_LIMIT aligns with the introduction of configurable gas limits in the SynapseModule. This change clarifies the purpose of the constant in the context of the tests.
• 88-91: The test_requestVerification_accumulatesFee function correctly tests the accumulation of fees upon request verification. This test is essential for validating the fee collection mechanism's correctness.
• 101-104: The test_requestVerification_feeAboveRequired_accumulatesFee function ensures that fees above the required amount are correctly accumulated. This test case is important for verifying the contract's behavior when excess fees are provided.
• 114-120: The test_claimFees_zeroClaimFee_emitsEvent function tests the emission of the FeesClaimed event with a zero claim fee. This test is crucial for ensuring that the event is emitted correctly under different fee claiming scenarios.
• 122-128: The test_claimFees_zeroClaimFee_distributesFees function validates the distribution of fees when the claim fee is zero. This test ensures that all fees are transferred to the feeCollector as expected.
• 130-136: The test_claimFees_zeroClaimFee_revert_feeCollectorNotSet function correctly tests the revert condition when the feeCollector is not set. This test is important for ensuring robust error handling in the fee claiming process.
• 138-142: The test_claimFees_zeroClaimFee_revert_noFeesToClaim function tests the revert condition when there are no fees to claim. This test case is essential for validating the contract's behavior in scenarios where fee claiming is attempted without any accumulated fees.
• 144-153: The test_claimFees_nonZeroClaimFee_emitsEvent function tests the emission of the FeesClaimed event with a non-zero claim fee. This test is crucial for verifying the event's correctness when a portion of the fees is claimed by the caller.
• 155-163: The test_claimFees_nonZeroClaimFee_distributesFees function validates the fee distribution when a non-zero claim fee is set. This test ensures that both the feeCollector and the claimer receive their respective portions of the fees correctly.
• 187-208: The test_getClaimFeeAmount functions (for both zero and non-zero claim fees) correctly test the calculation of the claim fee amount under various conditions. These tests are important for ensuring the accuracy of the claim fee calculation logic.
• 215-259: The test_getModuleFee functions test the getModuleFee logic under various conditions, including default and custom gas limits, as well as different numbers of signers. These tests are essential for validating the module fee calculation and the interaction with the GasOracleMock.
• 261-263: The test_getVerifyGasLimit_default function correctly tests the retrieval of the default gas limit when no custom limit is set. This test is important for verifying the fallback to the default gas limit in the absence of a custom configuration.

packages/contracts-communication/test/modules/SynapseModule.Management.t.sol (6)

• 28-35: The addition of the allVerifiers array and its initialization in the setUp function is a good practice for setting up test conditions. This approach allows for easier management of test verifiers and supports the new functionality for adding and removing multiple verifiers.
• 65-97: The tests for adding a single verifier (test_addVerifier_addsVerifier, test_addVerifier_emitsEvent, test_addVerifier_revert_alreadyAdded, test_addVerifier_revert_notOwner) and multiple verifiers (test_addVerifiers_addsVerifiers, test_addVerifiers_emitsEvents, test_addVerifiers_revert_alreadyAdded, test_addVerifiers_revert_notOwner) are well-structured and cover important scenarios including success cases, event emissions, and revert conditions for already added verifiers and unauthorized access. These tests ensure the verifier addition functionality works as expected and is secure against unauthorized modifications.
• 126-160: Similar to the addition tests, the removal tests for both single (test_removeVerifier_removesVerifier, test_removeVerifier_emitsEvent, test_removeVerifier_revert_notAdded, test_removeVerifier_revert_notOwner) and multiple verifiers (test_removeVerifiers_removesVerifiers, test_removeVerifiers_emitsEvents, test_removeVerifiers_revert_notAdded, test_removeVerifiers_revert_notOwner) are comprehensive. They effectively validate the functionality for removing verifiers, ensuring the module behaves correctly in various scenarios including successful removals, event emissions, and handling unauthorized access and attempts to remove non-existent verifiers.
• 236-272: The tests for setting the fee fraction (test_setFeeFraction_setsFeeFraction, test_setFeeFraction_emitsEvent, test_setFeeFraction_exactlyMax, test_setFeeFraction_revert_exceedsMax, test_setFeeFraction_revert_notOwner) are thorough and cover a wide range of scenarios including setting to a valid value, emitting the correct event, handling maximum allowed value, reverting on exceeding the maximum, and ensuring only the owner can make changes. These tests are crucial for ensuring the fee fraction functionality is robust, secure, and adheres to specified constraints.
• 305-322: The addition of tests for setting the verify gas limit (test_setVerifyGasLimit_setsVerifyGasLimit, test_setVerifyGasLimit_emitsEvent, test_setVerifyGasLimit_revert_notOwner) is a valuable enhancement. These tests ensure that the gas limit for verification can be configured correctly, that the appropriate event is emitted upon change, and that only the owner has the authority to make this change. This functionality is essential for managing gas costs effectively in the verification process.
• 297-297: The test test_setGasOracle_revert_notContract includes a sanity check to ensure that the address provided for the gas oracle is indeed a contract. This is a good security practice to prevent setting a non-contract address as the gas oracle, which could lead to unexpected behavior or vulnerabilities. However, it's important to ensure that this check is also performed in the main contract code, not just in the test, to enforce this validation outside of testing environments.

Verification successful

---

The verification process has confirmed that the setGasOracle function in the SynapseModule.sol file includes a check to ensure that the provided address is indeed a contract. This validation is performed by checking if gasOracle_.code.length == 0 and reverting with a custom error if the condition is met, thereby enforcing the security practice outside of testing environments as well.

Scripts Executed

The following scripts were executed for the analysis:

---

Script:

#!/bin/bash
# Search for the setGasOracle function implementation in the SynapseModule contract to verify contract address validation.
rg "function setGasOracle" packages/contracts-communication/contracts/modules/SynapseModule.sol

Length of output: 164

---

---

Script:

#!/bin/bash
# Extract the full implementation of the setGasOracle function to check for contract address validation
rg -A 10 "function setGasOracle" packages/contracts-communication/contracts/modules/SynapseModule.sol

Length of output: 540

packages/contracts-communication/test/modules/SynapseModule.Destination.t.sol (3)

• 7-7: The direct import of InterchainEntry in this file, following the removal of its import from the SynapseModule declarations, is a necessary adjustment to maintain access to InterchainEntry functionalities within the tests. This change ensures that the tests remain functional and can directly utilize InterchainEntry for constructing test cases.
• 16-17: Disabling the solhint directives func-name-mixedcase and ordering at the beginning of the contract is a common practice in test files to accommodate the naming conventions and structure preferred in test scenarios. However, it's important to use these directives judiciously and ensure that the overall code quality and readability are not compromised. In this context, the usage seems appropriate given the conventions in Solidity testing frameworks.
• 4-20: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]

The overall structure and logic of the tests in this file appear to be well-designed, covering a range of scenarios for verifying entries with different signature conditions. The tests are crucial for ensuring the integrity and security of the entry verification process. It's important to continue maintaining and expanding these tests as the SynapseModule evolves to cover new functionalities and edge cases.

[coderabbitai]

Review Status

Actionable comments generated: 5

Configuration used: .coderabbit.yaml

Commits

Files that changed from the base of the PR and between dbb8b66 and f3e196e.

Files selected for processing (7)

• packages/contracts-communication/contracts/events/SynapseModuleEvents.sol (1 hunks)
• packages/contracts-communication/contracts/interfaces/ISynapseModule.sol (5 hunks)
• packages/contracts-communication/contracts/modules/InterchainModule.sol (2 hunks)
• packages/contracts-communication/contracts/modules/SynapseModule.sol (6 hunks)
• packages/contracts-communication/test/modules/SynapseModule.Destination.t.sol (1 hunks)
• packages/contracts-communication/test/modules/SynapseModule.Management.t.sol (5 hunks)
• packages/contracts-communication/test/modules/SynapseModule.Source.t.sol (4 hunks)

Files skipped from review as they are similar to previous changes (4)

• packages/contracts-communication/contracts/events/SynapseModuleEvents.sol
• packages/contracts-communication/contracts/interfaces/ISynapseModule.sol
• packages/contracts-communication/contracts/modules/InterchainModule.sol
• packages/contracts-communication/test/modules/SynapseModule.Destination.t.sol

Additional comments: 10

packages/contracts-communication/contracts/modules/SynapseModule.sol (10)

• 17-18: The default gas limit of 100,000 for verifyEntry() is introduced. Ensure this value has been benchmarked against typical use cases to avoid setting a limit that's too low or unnecessarily high, potentially leading to failed transactions or wasted gas.
• 20-21: Constants MAX_CLAIM_FEE_FRACTION and FEE_PRECISION are well-defined. It's good practice to define such constants for clarity and maintainability. Ensure the maximum claim fee fraction of 1% is appropriate for the system's economic model.
• 25-28: The introduction of internal variables for claim fee fraction and gas limits is a positive change for flexibility and configurability. Ensure that access to these variables is properly restricted and validated where applicable.
• 81-85: The validation to ensure claimFeeFraction does not exceed MAX_CLAIM_FEE_FRACTION is crucial for maintaining system integrity. This check prevents setting an excessively high fee fraction. Good implementation.
• 97-100: Configurability of the verifyEntry function's gas limit per chain is a significant enhancement for flexibility. Ensure that there's a mechanism to prevent setting excessively low gas limits that could cause transactions to fail.
• 105-118: The claimFees function's implementation, including the calculation of claimFee and transferring fees to both the feeCollector and the caller, is well thought out. Ensure that the contract has a mechanism to receive and hold ETH for this functionality to work as expected.
• 129-132: Exposing the claim fee fraction through a public getter is a good practice for transparency. This allows external parties to easily retrieve the current fee fraction.
• 145-147: The calculation of the claim fee amount using the contract's balance and the claim fee fraction is correct. Ensure that rounding errors are considered and have a negligible impact on the system's economics.
• 155-159: The fallback to DEFAULT_VERIFY_GAS_LIMIT when a specific chain's gas limit is not set is a sensible default behavior. This ensures that the system remains operational even if specific configurations are missing.
• 188-188: The _getModuleFee function's implementation for estimating transaction costs on remote chains is a complex but necessary feature. Ensure that the gas oracle and calldata size calculations are accurate to prevent overestimating or underestimating fees.