LF-12584 - conventions.md

conventions.md

@@ -1,23 +1,23 @@
 # Repository overview
 
-- **Project name:** LiFi  
-- **Purpose:** LiFi is a cross-chain bridge aggregation protocol that ensures secure and efficient interoperability through robust smart contracts and automated processes.  
+- **Project name:** LI.FI  
+- **Purpose:** LI.FI is a cross-chain bridge aggregation protocol that ensures secure and efficient interoperability through robust smart contracts and automated processes.  
 - **Core components:**
   - **Smart contracts:** Primarily built using the Diamond Standard (EIP-2535) with modular facets for core functionality, along with supporting periphery contracts for extended features and integrations.
-  - **Automation scripts:** Deployment, migration, and operational tasks.
+  - **Automation scripts:** Deployment, updating, and operational tasks.
   - **Tests:** Tests ensuring contract reliability and safety.
   - **Documentation:** Detailed guides, API specifications, and deployment instructions.
 
 # Codebase structure
 
     /lifi
-     ├── src/               # Solidity smart contracts (Diamond facets + periphery) - `.sol` files
+     ├── src/               # All Solidity smart contracts - `.sol` files
      ├── tasks/             # Utility scripts or tasks
-     ├── scripts/           # Deployment, migration, and automation scripts
+     ├── scripts/           # Deployment, updating, and automation scripts
      ├── tests/             # Unit and integration tests - `.t.sol` files
      ├── docs/              # Project documentation, API specs, and guides
      ├── .github/           # Github workflows
-     ├── config/            # Facets' configuration files
+     ├── config/            # Configuration files
      ├── deployments/       # Deployment logs and addresses
      ├── audit/             # Audit reports and log
      ├── README.md          # High-level project overview and setup instructions
@@ -30,52 +30,114 @@ Follow the folder structure to locate resources and generate or modify code in a
 ## Solidity standards and patterns
 
 - **Solidity version:**  
-  All Solidity files must start with:
-
-    pragma solidity ^0.8.17;
+  All Solidity files must start with: pragma solidity {version}. The Solidity version is defined in foundry.toml. This ensures that the version declaration in the source files remains in sync with the configuration. Always refer to foundry.toml for the current version rather than storing duplicate information.
 
 - **Design patterns:**  
-  - Use established patterns (e.g., Ownable for access control, Diamond Standard for facets).  
+  - Use established patterns (e.g., Ownable for access control, EIP-2535 Diamond Standard for facet-based architectures).  
   - Favor modular design to enhance reusability and security.
 - **Security best practices:**  
-  - Validate inputs using `require` statements.  
   - Validate constructor inputs rigorously: if an invalid value (e.g., `address(0)` or zero value) is provided, revert with a custom error such as `InvalidConfig`. Ensure tests cover these conditions.  
   - Utilize reentrancy guards (e.g., OpenZeppelin’s `ReentrancyGuard` or the checks-effects-interactions pattern).  
   - Optimize for gas efficiency with proper data structures and minimal state changes.
 
 ## Facet contract checklist
-
+- All facet contracts must reside in `src/Facets/`.
+- Facet contract names must include the word `Facet`.
 - Facets must always include the following three functions:
   1. `_startBridge` – an internal function.
   2. `swapAndStartBridgeTokensVia{FacetName}`.
   3. `startBridgeTokensVia{FacetName}`.
+- **General inheritance**
+  Facet contracts typically inherit from a set of core contracts/interfaces to ensure standardized functionality. A typical facet contract inherits from:
+  - **Interfaces**:
+    - `ILiFi` - Defines the standard interface for all bridging operations.
+  - **Libraries**:
+    - `LibAsset` - Handles asset deposits, approvals, and transfers.
+    - `LibSwap` - Provides swap-related utilities.
+    - `LibAllowList` - Ensures only approved contract calls can be executed.
+  - **Security and utility contracts**:
+    - `ReentrancyGuard` - Protects against reentrancy attacks.
+    - `SwapperV2` - Provides swapping functionality.
+    - `Validatable` - Implements bridge data validation logic.
+  - **External utility contracts**:
+    - `ECDSA` - Used for signature verification.
 - **Sender handling:**  
-  Confirm whether `msg.sender` is justified. Often, pass the “sender/depositor” as a parameter so refunds return directly to the user.
+  Confirm whether the use of msg.sender is justified. Especially if a facet needs to pass an address for refunds (named `refundAddress` or `depositor` or similar) to the bridge itself, we discourage the use of msg.sender and instead recommend to use a dedicated parameter so the refundAddress can be specified independently of who sends/executes the transaction.
+
+  Often, pass the “sender/depositor” as a parameter so refunds return directly to the user.
 - **Parameter adjustments:**  
-  After a swap, verify if facet-specific parameters (e.g., expected `outputAmount`) require adjustment based on the actual swap outcome.
+  After a swap, verify if facet-specific parameters (e.g., expected `outputAmount`) require adjustment based on the actual swap outcome. This is especially relevant for intent-based bridges, where execution paths may vary, leading to deviations in expected amounts.
 - **Parameter ordering:**  
-  For facets with a `receiverAddress` parameter, it should be the first parameter in the `facetData` struct and must match the `bridgeData.receiver`.
+  For facets with a `receiverAddress` parameter, it should be the first parameter in the `{facetName}Data` struct.
+  The receiverAddress must always be explicitly validated against `bridgeData.receiver` in the function logic:
+  ```
+  if (
+      (bridgeData.receiver != {facetName}Data.receiverAddress)
+  ) revert InformationMismatch();
+  ```
+  This ensures that funds are always sent to the correct recipient and prevents potential misrouting due to incorrectly passed parameters.
 - **Cross-verification:**  
-  If `facetData` contains a `targetChainId`, verify it against `bridgeData.destinationChain`.
-- **Modifiers and events:**  
-  Ensure usage of default modifiers (e.g., `nonReentrant`, `refundExcessNative`) and that the `LiFiTransferStarted` event is emitted.
-- **Fee handling and non-evm support:**  
-  For native fees, use the `_depositAndSwap` variant that reserves the fee. For non-evm chains (e.g., Bitcoin), ensure the `receiverAddress` is declared as `bytes` (not `bytes32`).
-
-## Solidity tests conventions (.t.sol files)
+  If `facetData` contains a `targetChainId`, the facet must verify it against `bridgeData.destinationChain` to ensure these values match. This check applies only to EVM-to-EVM transactions.
+- **Modifiers:**  
+  Each facet must properly use the following modifiers:
+  - `nonReentrant` - Prevents reentrancy attacks by ensuring that functions cannot be called recursively within the same transaction.
+  - `refundExcessNative` - Ensures that any excess native tokens sent to the contract are refunded to the user after execution.
+  - `validateBridgeData` - Ensures that the bridgeData is valid, including nonzero addresses, positive amounts, and a different destination chain.
+  - `doesNotContainSourceSwaps` / `doesContainSourceSwaps` - Enforces whether source swaps are expected or not, preventing unintended swaps.
+  - `doesNotContainDestinationCalls` / `doesContainDestinationCalls` - Ensures that destination calls are only included when expected, avoiding accidental post-bridge execution.
+
+- **Events:**  
+  1. Transaction Start:
+      - Event: `LiFiTransferStarted`
+      - When to emit: At the start of a successful transaction, before any external calls.
+      - Purpose: Enables tracking of bridge transactions.
+      ```
+      emit LiFiTransferStarted(bridgeData);
+      ```
+
+  2. Transaction Completion:
+      - Event: `LiFiTransferCompleted`
+      - When to emit: After a transaction is successfully executed and the funds have arrived at the destination.
+      - Purpose: Signals that the bridge transaction was finalized.
+
+  3. Transaction Recovery:
+      - Event: `LiFiTransferRecovered`
+      - When to emit: If a transaction fails or is refunded and the original sender recovers their funds.
+      - Purpose: Ensures visibility for recoveries and refunds.
+
+- **Fee handling:**  
+  For native fees, the facet must use the `_depositAndSwap` variant that reserves the required fee before execution. This ensures that native assets needed for bridging are deducted correctly.
+- **Non-EVM chain support:**  
+  For transactions targeting non-EVM chains (e.g., Bitcoin, Solana):
+    - The `receiverAddress` must be declared as `bytes` (not `bytes32`).
+    - The facet must ensure that the non-EVM address is not zero, enforcing this with:
+      ```
+      if ({facetName}Data.receiverAddress == bytes32(0)) {
+          revert InvalidNonEVMReceiver(
+              {facetName}Data.receiverAddress,
+              bytes32(0)
+          );
+      }
+      ```
+    - When a non-EVM address is used, the `bridgeData.receiver` must contain `LibAsset.NON_EVM_ADDRESS`, ensuring proper handling:
+      ```
+      if (
+          bridgeData.receiver != LibAsset.NON_EVM_ADDRESS
+      ) {
+          revert InvalidCallData();
+      }
+      ```
+
+
+## Solidity test conventions (.t.sol files)
 
 - **File naming and structure**  
-  - Test files typically have a `.t.sol` extension (e.g., `AcrossFacetV3.t.sol`, `DeBridgeDlnFacet.t.sol`, `DexManagerFacet.t.sol`).  
-  - Each file should begin with the SPDX license identifier and the Solidity version:
-
-        // SPDX-License-Identifier: Unlicense
-        pragma solidity ^0.8.17;
-
+  - Test files must have a `.t.sol` extension (e.g., `AcrossFacetV3.t.sol`, `DeBridgeDlnFacet.t.sol`, `DexManagerFacet.t.sol`).
   - Group and order imports with system libraries first and project files next.
 
 - **Test function naming**  
-  - Prefix test functions expected to pass with `test_`.  
-  - Prefix test functions expected to revert with `testRevert_` (using `vm.expectRevert` to check for specific error selectors).  
+  - All tests that verify a successful execution (i.e., not expecting any reverts) must be prefixed with: `test_`
+  - All tests that verify a failure case and expect a specific revert reason must be prefixed with: `testRevert_`. All negative tests must check for a specific revert reason. Usage of `vm.expectRevert()` without a reason is discouraged and should only be used in exceptional cases where the revert reason is dynamic.
   - Use clear and descriptive names that capture the test’s purpose. For example:  
     1. `test_CanSwapAndBridgeTokensWithOutputAmountPercent`  
     2. `testRevert_FailsIfCalledWithOutdatedQuote`  
@@ -86,7 +148,7 @@ Follow the folder structure to locate resources and generate or modify code in a
 - **Test structure and setup**  
   - Every test contract must include a `setUp()` function to initialize the test environment.  
   - The `setUp()` function typically configures custom block numbers, initializes base contracts, sets up facets, and assigns labels (using `vm.label`) for clarity.  
-  - Common initialization steps include calling `initTestBase()` and setting facet addresses in the test base.  
+  - Any contract that inherits from `TestBase.sol` must call `initTestBase()` in `setUp()` and set facet address.
   - Use `vm.startPrank(address)` and `vm.stopPrank()` to simulate transactions from different users.
 
 - **Assertions and event testing**  
@@ -95,13 +157,6 @@ Follow the folder structure to locate resources and generate or modify code in a
   - Before executing a function call that should emit an event, use `vm.expectEmit()` with appropriate flags (to check indexed parameters) and the expected event signature.  
   - Verify that the expected events (e.g., `AssetSwapped` or `LiFiTransferStarted`) are emitted with the intended parameters.
 
-- **Reversion and error handling**  
-  - Negative test cases should use `vm.expectRevert()` with the exact error selector (e.g., `InvalidQuoteTimestamp.selector`, `UnAuthorized.selector`).  
-  - The test name must reflect that the function is expected to fail (using the `testRevert_` prefix).  
-    - For example:
-      - `testRevert_FailsIfCalledWithOutdatedQuote` verifies that a call reverts when the quote timestamp is outdated.
-      - `testRevert_FailsIfNonOwnerTriesToAddDex` ensures that unauthorized addresses cannot perform owner-only actions.
-
 - **Overall test best practices**  
   - Include comments where necessary to explain the purpose of tests or to clarify non-obvious behavior.  
   - Maintain a consistent order in function calls and assertions.  
@@ -169,12 +224,12 @@ Follow the folder structure to locate resources and generate or modify code in a
   - Extract common logic into helper files (e.g., `script/helperFunctions.sh`).
 - **Environment and configuration:**  
   - Always load environment variables from a `.env` or `config.sh` file and configuration parameters from a dedicated config file.  
-  - Remember to update `.env.example` or `config.example.sh` accordingly.  
   - Declare global variables (like `CONTRACT_DIRECTORY`, `DEPLOY_SCRIPT_DIRECTORY`, etc.) in the `.env` or `config.sh` files so they are consistently available across all functions.
+  - Remember to update `.env.example` or `config.example.sh` accordingly.  
 - **Error handling and logging:**  
-  - Use dedicated helper functions for logging (e.g., `echoDebug`, `error`, `warning`, `success`) to provide consistent, color-coded feedback.  
+  - Use dedicated helper functions for logging (e.g., `echoDebug`, `error`, `warning`, `success`) stored in `script/helperFunctions.sh` to provide consistent, color-coded feedback.  
   - Validate external inputs and environment variables early in the script, and exit with a clear error message if required variables are missing.  
-  - **Child function failure checking:** When calling any underlying child function, immediately check its exit status using the `checkFailure` helper function. This ensures that if a function fails, the script halts further execution, preventing cascading errors.  
+  - When calling any underlying child function, immediately check its exit status using the `checkFailure` helper function. This ensures that if a function fails, the script halts further execution, preventing cascading errors.  
   - If installing a new system package, add it to `preinstall.sh` so that developers have a unified initial installation procedure.
 - **User interaction and prompts:**  
   - When user input is needed (e.g., selecting a network or confirming an action), use clear prompts with descriptive instructions. Tools like `gum choose` can enhance usability.  
@@ -188,23 +243,62 @@ Follow the folder structure to locate resources and generate or modify code in a
 
 - **Audit log file (`auditLog.json`):**  
   Contains two main sections:
-  1. **audits:** Each audit entry has a unique ID (e.g., `auditYYYYMMDD`), an auditCompletedOn date, auditedBy, auditorGitHandle (if applicable), auditReportPath (PDF location in `audit/reports/`), and an auditCommitHash.
+  1. **audits:** Each audit entry has a unique ID (e.g., `auditYYYYMMDD_X`) where `YYYYMMDD` = Audit completion date, `_X` = Incrementing counter for multiple audits on the same day. Ensure that contract versioning is consistent between the code and the `auditLog.json`.
+  - Essential fields for each audit entry:
+    - `auditCompletedOn`: Date in `DD.MM.YYYY` or `YYYY-MM-DD` format.  
+    - `auditedBy`: Name or firm.  
+    - `auditorGitHandle`: (if applicable).  
+    - `auditReportPath`: PDF location in `audit/reports/`.  
+    - `auditCommitHash`: The commit hash that was audited (or “n/a” if not tracked).
   2. **auditedContracts:** Maps contract names and versions to the relevant audit IDs in the audits section.
 - **Storing reports:**  
-  Place PDF reports in the `audit/reports/` directory, using a naming format that includes the date and contract info (e.g., `2025.01.17_LiFiDexAggregator(v1.3.0).pdf`).
+  Place PDF reports in the `audit/reports/` directory. The naming format must follow: `YYYY.MM.DD_ContractName(version).pdf` (e.g., `2025.01.17_LiFiDexAggregator(v1.3.0).pdf`).
 - **Adding new audits:**  
   For new or updated contracts, add an entry under audits with the correct date, auditReportPath, and auditCommitHash. Then, reference that new ID in auditedContracts for each relevant version.
-- **Naming and versioning:**  
-  Use a format like `auditYYYYMMDD` for unique IDs and ensure that contract versioning is consistent between the code and the `auditLog.json`.
-- **Essential fields:**  
-  - `auditCompletedOn`: Date in `DD.MM.YYYY` or `YYYY-MM-DD` format.  
-  - `auditedBy`: Name or firm.  
-  - `auditorGitHandle`: (if applicable).  
-  - `auditReportPath`: PDF location in `audit/reports/`.  
-  - `auditCommitHash`: The commit hash that was audited (or “n/a” if not tracked).
 
 # Documentation and references
 
 - **Primary documentation:**  
   - `README.md`: Contains an overview and setup instructions.  
-  - `/docs`: Contains detailed technical documentation, API specifications, and deployment guides.
+  - `/docs`: Contains detailed technical documentation, API specifications, and deployment guides.
+
+# Deployment and update scripts
+
+Deployment and update scripts for LI.FI smart contracts are located in: `script/deploy/facets/`. Each contract has a corresponding `Deploy` and `Update` script. These scripts ensure that contract deployments and upgrades follow a structured and consistent approach.
+
+
+- **Naming conventions:**  
+  - Deployment scripts must be prefixed with `Deploy` followed by the contract name (e.g., zDeployMayanFacet.s.solz).
+  - Update scripts must be prefixed with `Update` followed by the contract name (e.g., zUpdateMayanFacet.s.solz).
+- **Structure of Deployment scripts:**  
+  - Each deployment script follow this format:
+    - Inherits `DeployScriptBase` to maintain consistency.
+    - Uses JSON config (`stdJson`) to fetch contract-specific configuration data.
+    - Defines `getConstructorArgs()` to handle constructor arguments dynamically.
+    - Encodes constructor arguments before deployment.
+    - Calls `deploy()` using `type({ContractName}).creationCode`.
+- **Structure of Update scripts:**  
+  - Each deployment script follow this format:
+    - Inherits `UpdateScriptBase` for consistency in update logic.
+    - Calls `update("{ContractName}")` to handle facet upgrades in the Diamond architecture.
+    - Ensures correct function selectors are updated.
+  - Special Case: Some facets may require the exclusion of certain function selectors during updates. This is handled using `getExcludes()`:
+    - Define an instance of the contract (`contractInstance`).
+    - Use `.selector` to exclude specific functions.
+    - Return an array containing function selectors to be excluded.
+- **Configuration and JSON handling:**  
+  - Each deployment script references JSON config files under: `/config/`
+    - The script dynamically selects values based on the network using: 
+      ```
+      string memory path = string.concat(root, "/config/{facetName}.json");
+      address {configValueVariableName} = _getConfigContractAddress(
+        path,
+        string.concat(".{key}.", network, ".{subkey}")
+      );
+      ```
+      This allows fetching various values such as bridges, allowed tokens, and other necessary configurations dynamically based on the network and facet needs.
+
+
+
+
+