LF-12584 - conventions.md #1046
Conversation
WalkthroughThis pull request updates the documentation in Changes
Possibly related PRs
Suggested reviewers
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
🪧 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
conventions.md (4)
64-67: Sender Handling Guidance:
The recommendation to avoid relying solely onmsg.senderin favor of a dedicated parameter is well-founded.
- Nitpick: Consider rephrasing for grammatical clarity, e.g., "we discourage the use of
msg.senderand recommend using a dedicated parameter…"- Additionally, adding a comma before "so" in line 67 may improve readability.
🧰 Tools
🪛 LanguageTool
[grammar] ~65-~65: The verb ‘recommend’ is used with the gerund form.
Context: ...urage the use of msg.sender and instead recommend to use a dedicated parameter so the refundAddr...(ADMIT_ENJOY_VB)
[uncategorized] ~67-~67: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...ss the “sender/depositor” as a parameter so refunds return directly to the user. - ...(COMMA_COMPOUND_SENTENCE_2)
71-79: Parameter Ordering & Validation Example:
The example provided for validating thereceiverAddressis precise and demonstrates best practices.
- Refactor Suggestion: Specify a language (e.g., ```solidity) in the fenced code block to conform with markdown linting rules and enhance readability.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
73-73: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
73-73: Code block style
Expected: indented; Actual: fenced(MD046, code-block-style)
90-112: Event Emission Guidelines:
The events section clearly outlines when and which events to emit, helping with transaction tracking and backward compatibility.
- Refactor Suggestion: As with other code examples, adding a language specifier (e.g., ```solidity) to the fenced code blocks would improve markdown clarity.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
95-95: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
95-95: Code block style
Expected: indented; Actual: fenced(MD046, code-block-style)
116-136: Non-EVM Chain Support Requirements:
This section provides a thorough and detailed approach for handling non-EVM chain addresses, including specific code examples and checks to prevent invalid addresses.
- Refactor Suggestion: Consider specifying the language in your fenced code blocks (for example, using ```solidity) to meet markdown best practices.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
conventions.md(6 hunks)
🧰 Additional context used
🪛 LanguageTool
conventions.md
[grammar] ~65-~65: The verb ‘recommend’ is used with the gerund form.
Context: ...urage the use of msg.sender and instead recommend to use a dedicated parameter so the refundAddr...
(ADMIT_ENJOY_VB)
[uncategorized] ~67-~67: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...ss the “sender/depositor” as a parameter so refunds return directly to the user. - ...
(COMMA_COMPOUND_SENTENCE_2)
[uncategorized] ~261-~261: Loose punctuation mark.
Context: ...r YYYY-MM-DD format. - auditedBy: Name or firm. - auditorGitHandle:...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~263-~263: Loose punctuation mark.
Context: ...(if applicable). - auditReportPath: PDF location in audit/reports/. -...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~264-~264: Loose punctuation mark.
Context: ...audit/reports/. - auditCommitHash`: The commit hash that was audited (or “n...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
conventions.md
73-73: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
73-73: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
95-95: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
95-95: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-unit-tests
🔇 Additional comments (14)
conventions.md (14)
3-4: Project Branding Update:
The project name and purpose have been updated consistently to "LI.FI." The rebranding is clear and uniformly applied.
7-7: Updated Automation Scripts Description:
Changing the description from "migration" to "updating" for automation scripts clarifies their broader functionality.
14-20: Enhanced Codebase Structure:
The updated folder structure—including the refined comments for thesrc/andscripts/directories and the addition of aconfig/directory—improves repository clarity and maintainability.
33-37: Solidity Version & Design Patterns Clarification:
The instruction to derive the Solidity version fromfoundry.tomland the emphasis on using established design patterns (including the EIP-2535 Diamond Standard) ensure a consistent development approach.
44-49: Facet Contract Checklist Addition:
Introducing a detailed checklist for facet contracts—covering storage location, naming conventions, and required functions—provides clear implementation guidelines for developers.
50-63: Inheritance Guidelines Enhancement:
The expanded section on inheritance now clearly defines the required interfaces, libraries, and security contracts. This comprehensive approach promotes standardization across facet contracts.
69-69: Parameter Adjustments Noted:
The note to verify whether facet-specific parameters (likeoutputAmount) need adjustments based on actual swap outcomes is clear and important for ensuring accurate bridge operations.
81-88: Modifiers and Cross-Verification:
The guidelines for verifying chain IDs and the comprehensive list of required modifiers are vital for ensuring robust contract behavior.
114-115: Fee Handling Explained Clearly:
The instructions for native fee handling, including the use of the_depositAndSwapvariant, are clearly described and straightforward.
139-147: Solidity Test Conventions:
The updated naming conventions and file structure guidelines for Solidity tests (including the required file extension, SPDX header, and ordering of imports) are clear and will help maintain consistent test code quality.
151-152: Test Function Naming Guidelines:
The differentiation between tests for successful execution (test_) and those expecting a revert (testRevert_) is well articulated, ensuring clarity in test outcomes.
163-163: Test Setup Best Practices:
Mandating that contracts inheriting fromTestBase.solcallinitTestBase()in theirsetUp()function is an effective way to standardize test initialization.
240-242: Bash Script Configuration Updates:
The reminder to update.env.exampleorconfig.example.shand the emphasis on using dedicated helper functions for logging significantly improve the maintainability and clarity of bash scripts within the repository.
258-270: Audit Logs and Reports Conventions:
The detailed guidelines for audit logging—including unique ID formats, essential audit entry fields, and the organization of audit reports—are comprehensive and well structured, helping maintain audit traceability across updates.🧰 Tools
🪛 LanguageTool
[uncategorized] ~261-~261: Loose punctuation mark.
Context: ...rYYYY-MM-DDformat. -auditedBy: Name or firm. -auditorGitHandle:...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~263-~263: Loose punctuation mark.
Context: ...(if applicable). -auditReportPath: PDF location inaudit/reports/. -...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~264-~264: Loose punctuation mark.
Context: ...audit/reports/. -auditCommitHash`: The commit hash that was audited (or “n...(UNLIKELY_OPENING_PUNCTUATION)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
conventions.md (5)
65-67: Enhanced Sender Handling Guidelines
The updated text clearly discourages the use ofmsg.senderby recommending a dedicated parameter for refund addresses. To improve clarity and grammatical precision, consider the following diff:- 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. + we discourage the use of msg.sender and instead recommend using a dedicated parameter, so that the refundAddress can be specified independently of who sends/executes the transaction.This minor improvement enhances readability.
🧰 Tools
🪛 LanguageTool
[grammar] ~65-~65: The verb ‘recommend’ is used with the gerund form.
Context: ...urage the use of msg.sender and instead recommend to use a dedicated parameter so the refundAddr...(ADMIT_ENJOY_VB)
[uncategorized] ~67-~67: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...ss the “sender/depositor” as a parameter so refunds return directly to the user. - ...(COMMA_COMPOUND_SENTENCE_2)
71-77: Parameter Ordering and Validation Instructions
The instructions on ensuring thereceiverAddressis the first parameter in the{facetName}Datastruct—with a follow-up validation snippet—are well detailed.Nitpick: Consider specifying the language (e.g.,
solidity) for the fenced code block to align with Markdown lint guidelines.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
73-73: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
73-73: Code block style
Expected: indented; Actual: fenced(MD046, code-block-style)
89-107: Event Emission Guidelines
The expanded section on events (covering Transaction Start, Completion, and Recovery) is very detailed and will aid in proper event tracking throughout bridge transactions.Nitpick: To meet Markdown lint best practices, consider adding a language identifier (like
solidityortext) for the fenced code blocks.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
94-94: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
94-94: Code block style
Expected: indented; Actual: fenced(MD046, code-block-style)
111-130: Non-EVM Chain Support Instructions
This section thoroughly outlines how to handle non-EVM chains, including validations forreceiverAddress(declared asbytes) and checking againstLibAsset.NON_EVM_ADDRESS. The inclusion of code snippets assists in clarifying the expected checks.Nitpick: As with other blocks, specifying a language for these fenced code blocks would improve Markdown consistency.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
112-112: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
113-113: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
114-114: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
114-114: Code block style
Expected: indented; Actual: fenced(MD046, code-block-style)
122-122: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
123-123: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
123-123: Code block style
Expected: indented; Actual: fenced(MD046, code-block-style)
265-305: Deployment and Update Scripts Conventions
This new, extensive section provides detailed conventions on:
- Naming (e.g.,
Deploy/Updateprefixes),- Structure (inheritance from
DeployScriptBase/UpdateScriptBase),- Use of JSON configuration, and
- Handling of function selectors for facet upgrades.
These guidelines will greatly aid in maintaining consistency and reducing errors during contract deployments and updates.
Nitpick: For improved Markdown compliance, consider adding language identifiers (e.g.,
solidityfor code snippets) in the fenced code blocks within this section.🧰 Tools
🪛 LanguageTool
[grammar] ~274-~274: “Script” is a singular noun. It appears that the verb form is incorrect.
Context: ...scripts:** - Each deployment script follow this format: - Inherits `DeployScri...(PCT_SINGULAR_NOUN_PLURAL_VERB_AGREEMENT)
[grammar] ~281-~281: “Script” is a singular noun. It appears that the verb form is incorrect.
Context: ...scripts:** - Each deployment script follow this format: - Inherits `UpdateScri...(PCT_SINGULAR_NOUN_PLURAL_VERB_AGREEMENT)
🪛 markdownlint-cli2 (0.17.2)
292-292: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
292-292: Code block style
Expected: indented; Actual: fenced(MD046, code-block-style)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
conventions.md(5 hunks)
🧰 Additional context used
🪛 LanguageTool
conventions.md
[grammar] ~65-~65: The verb ‘recommend’ is used with the gerund form.
Context: ...urage the use of msg.sender and instead recommend to use a dedicated parameter so the refundAddr...
(ADMIT_ENJOY_VB)
[uncategorized] ~67-~67: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...ss the “sender/depositor” as a parameter so refunds return directly to the user. - ...
(COMMA_COMPOUND_SENTENCE_2)
[uncategorized] ~136-~136: Possible missing comma found.
Context: ...nagerFacet.t.sol`). - Group and order imports with system libraries first and project...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~249-~249: Loose punctuation mark.
Context: ...YYYY-MM-DD format. - auditedBy: Name or firm. - `auditorGitHandle...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~251-~251: Loose punctuation mark.
Context: ...f applicable). - auditReportPath: PDF location in audit/reports/. ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~252-~252: Loose punctuation mark.
Context: ...dit/reports/. - auditCommitHash`: The commit hash that was audited (or “n...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~263-~263: Loose punctuation mark.
Context: ...ew and setup instructions. - /docs: Contains detailed technical documentati...
(UNLIKELY_OPENING_PUNCTUATION)
[grammar] ~274-~274: “Script” is a singular noun. It appears that the verb form is incorrect.
Context: ...scripts:** - Each deployment script follow this format: - Inherits `DeployScri...
(PCT_SINGULAR_NOUN_PLURAL_VERB_AGREEMENT)
[grammar] ~281-~281: “Script” is a singular noun. It appears that the verb form is incorrect.
Context: ...scripts:** - Each deployment script follow this format: - Inherits `UpdateScri...
(PCT_SINGULAR_NOUN_PLURAL_VERB_AGREEMENT)
🪛 markdownlint-cli2 (0.17.2)
conventions.md
73-73: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
73-73: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
94-94: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
94-94: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
112-112: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
113-113: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
114-114: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
114-114: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
122-122: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
123-123: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
123-123: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
292-292: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
292-292: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: generate-tag
🔇 Additional comments (18)
conventions.md (18)
3-4: Rebranding Update: Project Name and Purpose
The updated project name (“LI.FI”) and revised purpose statement clearly reinforce the new branding and protocol description. Ensure that all future references across the repository use “LI.FI” consistently.
7-7: Clarification on Automation Scripts Description
The change from “migration” to “deployment, updating, and operational tasks” better reflects the broad functionality of the automation scripts. This adds clarity to the core components of the repository.
14-16: Codebase Structure Refinement
Updating the description for thesrc/directory to “All Solidity smart contracts” and clarifying that thescripts/directory now handles deployment, updating, and automation tasks improves overall clarity regarding the project’s folder structure.
33-33: Solidity Version Standardization
Defining that all Solidity files should start with a pragma using the version specified infoundry.tomlis a solid approach. This helps prevent version mismatches across the codebase.
44-45: Facet Contract Checklist Introduction
The addition of a checklist specifying that all facet contracts must reside insrc/Facets/and include “Facet” in their names is a great move toward enforcing consistency.
50-57: General Inheritance Guidelines Update
The detailed list of required interfaces and libraries (e.g.,ILiFi,LibAsset,LibSwap, andLibAllowList) sets a clear standard for facet contracts. This structured approach will help maintain uniform behavior across diverse contracts.
58-63: Security & External Dependency Inheritance
Including security and utility contracts—such asReentrancyGuard,SwapperV2, andValidatable—along with external utilities likeECDSA, enhances clarity on the expected inheritance pattern and reinforces security best practices.
68-69: Parameter Adjustments Clarification
The guideline to verify if facet-specific parameters (such as the expectedoutputAmount) require adjustment based on actual swap outcomes is clearly stated. This consideration is especially important for intent-based bridges where execution may vary.
79-80: Cross-Verification for Target Chain ID
Verifying thattargetChainIdinfacetDatamatchesbridgeData.destinationChainfor EVM-to-EVM transactions adds an important security check.
82-87: Modifiers Usage Requirements
The comprehensive list of required modifiers—with clear descriptions—ensures that facets implement proper protections (e.g., against reentrancy) and expected behavior (such as refunding excess native tokens).
109-109: Fee Handling Directive
The concise directive to use the_depositAndSwapvariant for native fee handling is straightforward and unambiguous.
135-136: Solidity Test File Naming and Structure
Requiring that test files end with a.t.solextension, as well as grouping imports logically, ensures consistent test organization.🧰 Tools
🪛 LanguageTool
[uncategorized] ~136-~136: Possible missing comma found.
Context: ...nagerFacet.t.sol`). - Group and order imports with system libraries first and project...(AI_HYDRA_LEO_MISSING_COMMA)
139-146: Test Function Naming Conventions
The clear rules on naming test functions (usingtest_,testRevert_, andtestBase_prefixes) will greatly enhance readability and maintainability of the test suite.
151-151: Test Setup Requirements
Mandating that contracts inheriting fromTestBase.solcallinitTestBase()in theirsetUp()function ensures consistent test initialization.
228-232: Bash Scripts: Environment and Error Handling Updates
The updates in the Bash scripts guidelines emphasize:
- Updating
.env.exampleorconfig.example.shaccordingly (line 228).- Using dedicated helper functions for logging (line 230).
- Immediately checking child function exit statuses with the
checkFailurehelper (line 232).These changes promote consistency and robust error handling in scripts.
246-252: Audit Log File Conventions Enhancement
The expanded guidance on audit log entries—including the unique ID format, essential fields (such asauditCompletedOn,auditedBy, etc.), and the required consistency with contract versioning—will help maintain a clear audit trail.🧰 Tools
🪛 LanguageTool
[uncategorized] ~249-~249: Loose punctuation mark.
Context: ...YYYY-MM-DDformat. -auditedBy: Name or firm. - `auditorGitHandle...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~251-~251: Loose punctuation mark.
Context: ...f applicable). -auditReportPath: PDF location inaudit/reports/. ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~252-~252: Loose punctuation mark.
Context: ...dit/reports/. -auditCommitHash`: The commit hash that was audited (or “n...(UNLIKELY_OPENING_PUNCTUATION)
255-255: Audit Report Storage Naming Convention
The specified naming format for storing audit reports (e.g.,YYYY.MM.DD_ContractName(version).pdf) is clear and should facilitate organized audit documentation.
263-264: Documentation References Update
Updating the reference for primary documentation to include/docsensures that detailed technical documentation, API specifications, and deployment guides are easily accessible.🧰 Tools
🪛 LanguageTool
[uncategorized] ~263-~263: Loose punctuation mark.
Context: ...ew and setup instructions. -/docs: Contains detailed technical documentati...(UNLIKELY_OPENING_PUNCTUATION)
Which Jira task belongs to this PR?
LF-12584
Why did I implement it this way?
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)