chore: remove unused code and clean up remappings #539
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request removes deprecated submodules, legacy paymaster scripts, testing utilities, and various token and faucet contracts. It eliminates files related to old deployment and oracle setups and updates import paths in swap contracts. Additionally, two new abstract contracts—one for ERC2771 meta-transaction support and its interface—have been introduced to standardize relayed transaction handling. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant F as Trusted Forwarder
participant C as Contract (ERC2771Recipient)
U->>F: Sends meta-transaction request
F->>C: Forwards request with appended sender info
C->>C: _msgSender() extracts original sender from calldata
C-->>U: Processes transaction based on true sender
Poem
🪧 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 (5)
contracts/src/ERC2771/IERC2771Recipient.sol (3)
18-18: Remove unnecessary 'view' modifier
The pipeline warns thatviewis not necessary here, likely because this function can be called without reading state.- function isTrustedForwarder(address forwarder) public virtual view returns(bool); + function isTrustedForwarder(address forwarder) public virtual returns(bool);🧰 Tools
🪛 GitHub Actions: Contracts
[warning] 18-18: Function 'isTrustedForwarder' should not have 'view' modifier as it is not necessary.
26-26: Remove unnecessary 'view' modifier
Similar to above,viewmight not be required if no state reading occurs.- function _msgSender() internal virtual view returns (address); + function _msgSender() internal virtual returns (address);🧰 Tools
🪛 GitHub Actions: Contracts
[warning] 26-26: Function '_msgSender' should not have 'view' modifier as it is not necessary.
35-35: Remove unnecessary 'view' modifier
As the pipeline suggests, trim theviewif it doesn't read from contract state.- function _msgData() internal virtual view returns (bytes calldata); + function _msgData() internal virtual returns (bytes calldata);🧰 Tools
🪛 GitHub Actions: Contracts
[warning] 35-35: Function '_msgData' should not have 'view' modifier as it is not necessary.
contracts/src/ERC2771/ERC2771Recipient.sol (2)
28-28: Remove theviewmodifier to address pipeline warnings.Although marking this function as
viewis standard for returning a stored state variable without mutation, the pipeline specifically warns against it. If you prefer to satisfy the pipeline’s guidelines, consider removing theviewmodifier:- function getTrustedForwarder() public virtual view returns (address forwarder){ + function getTrustedForwarder() public virtual returns (address forwarder){ return _trustedForwarder; }🧰 Tools
🪛 GitHub Actions: Contracts
[warning] 28-28: Function 'getTrustedForwarder' should not have 'view' modifier as it is not a required method to allow Recipients to trust multiple Forwarders.
42-42: Remove theviewmodifier on_msgSender()to comply with pipeline feedback.The pipeline warns that
_msgSender()should not be declared asview. While usingviewindicates no state modification, removing it will align with the pipeline’s requirement:- function _msgSender() internal override virtual view returns (address ret) { + function _msgSender() internal override virtual returns (address ret) { if (msg.data.length >= 20 && isTrustedForwarder(msg.sender)) { ... } ... }🧰 Tools
🪛 GitHub Actions: Contracts
[warning] 42-42: Function '_msgSender' should not have 'view' modifier as it is not necessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
.gitmodules(0 hunks)contracts/lib/gsn(0 hunks)contracts/script/AATokenPaymaster.sol(0 hunks)contracts/script/Gsn.sol(0 hunks)contracts/script/OnboardingPaymaster.sol(0 hunks)contracts/script/OracleTokenPaymaster.sol(0 hunks)contracts/script/TestingWbtc.sol(0 hunks)contracts/src/ERC2771/ERC2771Recipient.sol(1 hunks)contracts/src/ERC2771/IERC2771Recipient.sol(1 hunks)contracts/src/TestingErc20.sol(0 hunks)contracts/src/faucet/Erc20Minter.sol(0 hunks)contracts/src/paymasters/AccountAbstraction/AATokenPaymaster.sol(0 hunks)contracts/src/paymasters/AccountAbstraction/SafeTransferLib.sol(0 hunks)contracts/src/paymasters/OnboardingPaymaster.sol(0 hunks)contracts/src/paymasters/Oracle.sol(0 hunks)contracts/src/paymasters/OracleTokenPaymaster.sol(0 hunks)contracts/src/swap/Btc_Marketplace.sol(1 hunks)contracts/src/swap/Faucet.sol(0 hunks)contracts/src/swap/Marketplace.sol(1 hunks)contracts/src/swap/Wrapped.sol(0 hunks)contracts/test/OnboardingPaymaster.t.sol(0 hunks)
💤 Files with no reviewable changes (17)
- contracts/lib/gsn
- .gitmodules
- contracts/src/TestingErc20.sol
- contracts/src/paymasters/OracleTokenPaymaster.sol
- contracts/script/OnboardingPaymaster.sol
- contracts/src/paymasters/OnboardingPaymaster.sol
- contracts/script/AATokenPaymaster.sol
- contracts/src/swap/Faucet.sol
- contracts/src/swap/Wrapped.sol
- contracts/script/OracleTokenPaymaster.sol
- contracts/script/Gsn.sol
- contracts/src/paymasters/AccountAbstraction/AATokenPaymaster.sol
- contracts/src/paymasters/AccountAbstraction/SafeTransferLib.sol
- contracts/test/OnboardingPaymaster.t.sol
- contracts/script/TestingWbtc.sol
- contracts/src/paymasters/Oracle.sol
- contracts/src/faucet/Erc20Minter.sol
🧰 Additional context used
🪛 GitHub Actions: Contracts
contracts/src/ERC2771/IERC2771Recipient.sol
[warning] 18-18: Function 'isTrustedForwarder' should not have 'view' modifier as it is not necessary.
[warning] 26-26: Function '_msgSender' should not have 'view' modifier as it is not necessary.
[warning] 35-35: Function '_msgData' should not have 'view' modifier as it is not necessary.
contracts/src/ERC2771/ERC2771Recipient.sol
[warning] 28-28: Function 'getTrustedForwarder' should not have 'view' modifier as it is not a required method to allow Recipients to trust multiple Forwarders.
[warning] 42-42: Function '_msgSender' should not have 'view' modifier as it is not necessary.
🔇 Additional comments (5)
contracts/src/swap/Marketplace.sol (1)
6-6: Local import path update
Good move referencing the localERC2771Recipientinstead of relying on external paths. This enhances maintainability.contracts/src/swap/Btc_Marketplace.sol (1)
8-8: Consistent local reference
Updating the import path forERC2771Recipientensures consistency with the rest of the project.contracts/src/ERC2771/IERC2771Recipient.sol (3)
1-17: Introduction of ERC-2771 abstract interface
Declaring this interface is a good approach for defining relayed-transaction handling.
19-25: Clarity and documentation
The comments clearly explain how to handle relayed transactions. No issues found.
27-34: Well-documented approach for_msgData
The usage and explanation for stripping the sender bytes is clear. Looks good.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
docs/docs/contracts/src/src/ERC2771/IERC2771Recipient.sol/abstract.IERC2771Recipient.md (2)
4-4: Use concise wording and ensure plural correctness.Consider rephrasing this sentence to avoid the phrase "in order to" and to use the plural "transactions," e.g.:
"A contract must implement this interface to support relayed transactions."
🧰 Tools
🪛 LanguageTool
[style] ~4-~4: Consider a shorter alternative to avoid wordiness.
Context: ... contract must implement this interface in order to support relayed transaction. It is rec...(IN_ORDER_TO_PREMIUM)
43-43: Add a comma after 'Otherwise'.To make the sentence clearer, add a comma after "Otherwise," as recommended by style checks:
🧰 Tools
🪛 LanguageTool
[uncategorized] ~43-~43: A comma might be missing here.
Context: ... call. For a call that came through the Forwarder the real sender is extracted from the l...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~43-~43: A comma may be missing after the conjunctive/linking adverb ‘Otherwise’.
Context: ...om the last 20 bytes of themsg.data. Otherwise simply returnsmsg.sender.| ### _ms...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
contracts/src/ERC2771/IERC2771Recipient.sol (2)
7-7: Shorten "in order to" and fix singular usage of "transaction".This doc sentence can be made more concise and grammatically accurate if modified as follows:
22-24: Add missing commas for clarity.In this description, a comma after “Forwarder” and “Otherwise” helps readability:
contracts/src/ERC2771/ERC2771Recipient.sol (2)
20-20: Consider zero-address checks or events when setting _trustedForwarder.Currently,
_trustedForwarderis private and there’s no guard against setting it to the zero address. Additionally, tracking changes on-chain might be easier if you emit an event when_trustedForwarderchanges.
31-33: Emit an event when assigning a new trusted forwarder.To aid debugging and auditing, it is a common pattern to emit an event whenever a privileged piece of state (like the trusted forwarder) changes:
+ // Emitting an event can help track forwarder changes on-chain + // emit TrustedForwarderChanged(_trustedForwarder, _forwarder); _trustedForwarder = _forwarder; }docs/docs/contracts/src/src/ERC2771/ERC2771Recipient.sol/abstract.ERC2771Recipient.md (2)
7-12: Grammar Improvement in Description:
On line 9, consider revising"A base contract to be inherited by any contract that want to receive relayed transactions."
to
"A base contract to be inherited by any contract that wants to receive relayed transactions."
This ensures proper subject-verb agreement.
68-82: Improve Clarity and Punctuation in_msgSenderDocumentation:
The descriptive text for_msgSendercould be enhanced for clarity and punctuation. For example, consider updating the line:"Use this method the contract anywhere instead of msg.sender to support relayed transactions."
to
"Use this method anywhere in the contract, instead ofmsg.sender, to support relayed transactions."Additionally, in the return description (line 80), adding a comma after "Otherwise" would improve readability:
"Otherwise simply returns
msg.sender."
should be updated to
"Otherwise, simply returnsmsg.sender."Diff Suggestions:
-Use this method the contract anywhere instead of msg.sender to support relayed transactions. +Use this method anywhere in the contract, instead of `msg.sender`, to support relayed transactions.-Otherwise simply returns `msg.sender`. +Otherwise, simply returns `msg.sender`.🧰 Tools
🪛 LanguageTool
[uncategorized] ~70-~70: Possible missing comma found.
Context: ... Recipient.| ### _msgSender Use this method the contract anywhere instead of msg.se...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~80-~80: Possible missing comma found.
Context: ... call. For a call that came through the Forwarder the real sender is extracted from the l...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~80-~80: A comma may be missing after the conjunctive/linking adverb ‘Otherwise’.
Context: ...om the last 20 bytes of themsg.data. Otherwise simply returnsmsg.sender.| ### _ms...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
.gitmodules(0 hunks)contracts/foundry.toml(1 hunks)contracts/lib/account-abstraction(0 hunks)contracts/remappings.txt(0 hunks)contracts/src/ERC2771/ERC2771Recipient.sol(1 hunks)contracts/src/ERC2771/IERC2771Recipient.sol(1 hunks)contracts/test/gateway/unit-tests/AvalonStrategy.sol(1 hunks)contracts/test/gateway/unit-tests/BedrockStrategy.sol(1 hunks)contracts/test/gateway/unit-tests/IonicStrategy.sol(1 hunks)contracts/test/gateway/unit-tests/PellStrategy.sol(1 hunks)contracts/test/gateway/unit-tests/SegmentStrategy.sol(1 hunks)contracts/test/gateway/unit-tests/ShoebillStrategy.sol(1 hunks)contracts/test/gateway/unit-tests/Utils.sol(1 hunks)contracts/test/swap/Btc_Marketplace.t.sol(1 hunks)contracts/test/swap/Marketplace.t.sol(1 hunks)contracts/test/swap/Ord_Marketplace.t.sol(1 hunks)docs/docs/contracts/src/src/ERC2771/ERC2771Recipient.sol/abstract.ERC2771Recipient.md(1 hunks)docs/docs/contracts/src/src/ERC2771/IERC2771Recipient.sol/abstract.IERC2771Recipient.md(1 hunks)docs/docs/contracts/src/src/paymasters/Oracle.sol/contract.DummyOracle.md(1 hunks)docs/docs/contracts/src/src/swap/Btc_Marketplace.sol/contract.BtcMarketPlace.md(1 hunks)docs/docs/contracts/src/src/swap/Marketplace.sol/contract.MarketPlace.md(1 hunks)
💤 Files with no reviewable changes (3)
- contracts/remappings.txt
- contracts/lib/account-abstraction
- .gitmodules
✅ Files skipped from review due to trivial changes (10)
- docs/docs/contracts/src/src/paymasters/Oracle.sol/contract.DummyOracle.md
- contracts/test/gateway/unit-tests/BedrockStrategy.sol
- contracts/test/gateway/unit-tests/IonicStrategy.sol
- contracts/test/gateway/unit-tests/Utils.sol
- contracts/test/gateway/unit-tests/ShoebillStrategy.sol
- contracts/test/gateway/unit-tests/AvalonStrategy.sol
- contracts/test/gateway/unit-tests/SegmentStrategy.sol
- docs/docs/contracts/src/src/swap/Marketplace.sol/contract.MarketPlace.md
- docs/docs/contracts/src/src/swap/Btc_Marketplace.sol/contract.BtcMarketPlace.md
- contracts/test/gateway/unit-tests/PellStrategy.sol
🧰 Additional context used
🪛 LanguageTool
docs/docs/contracts/src/src/ERC2771/IERC2771Recipient.sol/abstract.IERC2771Recipient.md
[style] ~4-~4: Consider a shorter alternative to avoid wordiness.
Context: ... contract must implement this interface in order to support relayed transaction. It is rec...
(IN_ORDER_TO_PREMIUM)
[uncategorized] ~43-~43: A comma might be missing here.
Context: ... call. For a call that came through the Forwarder the real sender is extracted from the l...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~43-~43: A comma may be missing after the conjunctive/linking adverb ‘Otherwise’.
Context: ...om the last 20 bytes of the msg.data. Otherwise simply returns msg.sender.| ### _ms...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
docs/docs/contracts/src/src/ERC2771/ERC2771Recipient.sol/abstract.ERC2771Recipient.md
[uncategorized] ~70-~70: Possible missing comma found.
Context: ... Recipient.| ### _msgSender Use this method the contract anywhere instead of msg.se...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~80-~80: Possible missing comma found.
Context: ... call. For a call that came through the Forwarder the real sender is extracted from the l...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~80-~80: A comma may be missing after the conjunctive/linking adverb ‘Otherwise’.
Context: ...om the last 20 bytes of the msg.data. Otherwise simply returns msg.sender.| ### _ms...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
🔇 Additional comments (10)
contracts/test/swap/Marketplace.t.sol (1)
6-7: Import statement updates look good.The import statements have been updated to use the standardized path format
@openzeppelin/contracts/and added theIERC20interface import. This aligns with the PR objective of removing unused code and updating import paths.contracts/test/swap/Ord_Marketplace.t.sol (1)
6-7: Import statement updates look good.The import statements have been updated to use the standardized path format
@openzeppelin/contracts/and added theIERC20interface import. This aligns with the PR objective of removing unused code and updating import paths.contracts/test/swap/Btc_Marketplace.t.sol (1)
6-7: Import statement updates look good.The import statements have been updated to use the standardized path format
@openzeppelin/contracts/and added theIERC20interface import. This aligns with the PR objective of removing unused code and updating import paths.contracts/foundry.toml (1)
12-16:❓ Verification inconclusive
Review Remappings Update
The new remappings block now includes only three entries:
@bob-collective/bitcoin-spv/=lib/bitcoin-spv/src/forge-std/=lib/forge-std/src/@openzeppelin/=lib/openzeppelin-contracts/Please verify that the removal of the previous remappings (which included entries for
ds-testand an additional OpenZeppelin path) does not break any import resolution or dependency expectations in other parts of the project. Double-check that no contracts or tests rely on the removed paths.
Below is a shell script we can run to search the repository for any references to
ds-testand the potentially removed OpenZeppelin remapping paths (for example, references to"openzeppelin/contracts/"). This will help us verify that removing the previously defined mappings doesn’t introduce any issues:Please execute the above script so we can verify if any dependencies on the removed remappings still exist.
🏁 Script executed:
#!/bin/bash echo "Searching for references to ds-test in the repository..." rg -n "ds-test" . echo "Searching for references to additional OpenZeppelin remapping (e.g., 'openzeppelin/contracts/')..." rg -n "openzeppelin/contracts" .Length of output: 11395
Review Remappings Impact – Verify Dependency Resolutions
The updated remappings block now includes only the following entries:
@bob-collective/bitcoin-spv/=lib/bitcoin-spv/src/forge-std/=lib/forge-std/src/@openzeppelin/=lib/openzeppelin-contracts/However, our repository search still shows references to ds-test (e.g., in
contracts/test/swap/Utilities.solimporting"ds-test/test.sol") alongside multiple OpenZeppelin imports. Please verify that:
- DS-Test: The removal of a dedicated ds-test remapping is intentional. Ensure that ds-test’s resolution is handled correctly (either via Builtin Foundry defaults or another configuration) so that tests relying on it continue to compile and run correctly.
- OpenZeppelin: The extra OpenZeppelin remapping that was removed does not affect contracts or documentation that import OpenZeppelin modules via the new remapping.
Kindly double-check that these changes do not disrupt any dependency or import resolution expectations across the project.
contracts/src/ERC2771/ERC2771Recipient.sol (1)
41-52: Add unit tests for _msgSender inline assembly logic.While the inline assembly approach is valid, thorough testing is critical to ensure correctness when extracting the sender from
msg.data. Consider adding specific test cases that:
• Verify the extracted sender when using a trusted forwarder.
• Confirm fallback behavior (i.e.,msg.senderusage) for direct calls or untrusted forwarders.Would you like me to generate a shell script or additional test scaffolding for verifying usage across the codebase?
docs/docs/contracts/src/src/ERC2771/ERC2771Recipient.sol/abstract.ERC2771Recipient.md (5)
22-32: Clear Documentation forgetTrustedForwarder:
The function description—including the warning about forwarder trust—is clear and well-structured. No changes needed here.
40-45: Documentation for_setTrustedForwarder:
The internal function's documentation is concise and clear.
47-54: Documentation forisTrustedForwarder:
The warning and the function signature are clearly presented. Good job emphasizing the risks associated with trusting a forwarder.
55-66: Return Value Documentation:
The table clearly outlines the return value forisTrustedForwarder. The layout is neat and informative.
83-96: Documentation for_msgData:
The explanation for_msgDatais clear and correctly describes how the appended sender address is handled when the call is relayed.
Orland0x
changed the title
Orland0x
force-pushed
the
chore/remove-unused-contracts
branch
from
96cff49 to
3cde3b2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
docs/docs/contracts/src/src/ERC2771/ERC2771Recipient.sol/abstract.ERC2771Recipient.md (5)
22-28: Revise Warning Message Grammar forgetTrustedForwarder
The warning message currently reads::warning: **Warning** :warning: The Forwarder can have a full control over your Recipient. Only trust verified Forwarder.Suggestions:
- Remove the article “a” before “full control” (i.e. “have full control”).
- Insert an article before “verified Forwarder” to read “Only trust a verified Forwarder.”
A suggested diff is:
-:warning: **Warning** :warning: The Forwarder can have a full control over your Recipient. Only trust verified Forwarder. +:warning: **Warning** :warning: The Forwarder can have full control over your Recipient. Only trust a verified Forwarder.🧰 Tools
🪛 LanguageTool
[uncategorized] ~26-~26: Possible missing article found.
Context: ...ipient. Only trust verified Forwarder. Method is not a required method to allow Recip...(AI_HYDRA_LEO_MISSING_THE)
47-54: Revise Warning Message Grammar forisTrustedForwarder
The warning for theisTrustedForwarderfunction repeats the same grammatical issues as ingetTrustedForwarder. Consider updating the message to::warning: **Warning** :warning: The Forwarder can have full control over your Recipient. Only trust a verified Forwarder.A suggested diff is:
-:warning: **Warning** :warning: The Forwarder can have a full control over your Recipient. Only trust verified Forwarder. +:warning: **Warning** :warning: The Forwarder can have full control over your Recipient. Only trust a verified Forwarder.
68-72: Clarify Usage Description for_msgSender
The sentence:Use this method the contract anywhere instead of msg.sender to support relayed transactions.could be clarified to improve readability. For example:
Use this method in your contract instead of directly using `msg.sender` to support relayed transactions.
76-82: Enhance Return Description Punctuation for_msgSender
In the returns table, consider adding a comma after “Otherwise” for better readability. The current text:For a call that came through the Forwarder the real sender is extracted from the last 20 bytes of the `msg.data`. Otherwise simply returns `msg.sender`.can be improved as:
For a call that came through the Forwarder, the real sender is extracted from the last 20 bytes of the `msg.data`. Otherwise, simply returns `msg.sender`.A diff suggestion:
-... extracted from the last 20 bytes of the `msg.data`. Otherwise simply returns `msg.sender`. +... extracted from the last 20 bytes of the `msg.data`. Otherwise, simply returns `msg.sender`.🧰 Tools
🪛 LanguageTool
[uncategorized] ~80-~80: A comma might be missing here.
Context: ... call. For a call that came through the Forwarder the real sender is extracted from the l...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~80-~80: A comma may be missing after the conjunctive/linking adverb ‘Otherwise’.
Context: ...om the last 20 bytes of themsg.data. Otherwise simply returnsmsg.sender.| ### _ms...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
90-97: Refine Return Table Punctuation for_msgData
In the returns table for_msgData, consider a similar punctuation adjustment for consistency. The relevant sentence:... - so this method will strip those 20 bytes off. Otherwise (if the call was made directly and not through the forwarder) simply returns `msg.data`.could be revised to:
... - so this method will strip those 20 bytes off. Otherwise, (if the call was made directly and not through the forwarder) simply returns `msg.data`.docs/docs/contracts/src/src/ERC2771/IERC2771Recipient.sol/abstract.IERC2771Recipient.md (4)
4-4: Simplify Interface Implementation Statement.Consider simplifying the wording to improve clarity and conciseness. For example, change:
"A contract must implement this interface in order to support relayed transaction."
to
"Implement this interface to support relayed transactions."
This aligns with concise documentation best practices.
🧰 Tools
🪛 LanguageTool
[style] ~4-~4: Consider a shorter alternative to avoid wordiness.
Context: ... contract must implement this interface in order to support relayed transaction. It is rec...(IN_ORDER_TO_PREMIUM)
12-13: Enhance Warning Message Clarity.The warning message is useful but could benefit from improved punctuation and clarity. For example, you might rephrase it to:
":warning: Warning
⚠️ The forwarder has full control over your recipient; only trust verified forwarders."This change improves readability without altering the intended caution.
31-33: _msgSender Documentation Rewording.The description for
_msgSendercould be streamlined. Consider revising it to:"Use this method throughout your contract instead of
msg.senderto correctly support relayed transactions."This rewording enhances clarity and flow.
41-43: Improve Return Description Punctuation in _msgSender.The return description for
_msgSendercan be improved for clarity by adjusting punctuation. For example, revise:-For a call that came through the Forwarder the real sender is extracted from the last 20 bytes of the `msg.data`. Otherwise simply returns `msg.sender`. +For a call that came through the Forwarder, the real sender is extracted from the last 20 bytes of `msg.data`, otherwise, `msg.sender` is returned.This adjustment addresses the static analysis suggestions regarding missing commas and enhances readability.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~43-~43: A comma might be missing here.
Context: ... call. For a call that came through the Forwarder the real sender is extracted from the l...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~43-~43: A comma may be missing after the conjunctive/linking adverb ‘Otherwise’.
Context: ...om the last 20 bytes of themsg.data. Otherwise simply returnsmsg.sender.| ### _ms...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
docs/docs/contracts/src/src/ERC2771/ERC2771Recipient.sol/abstract.ERC2771Recipient.md(1 hunks)docs/docs/contracts/src/src/ERC2771/IERC2771Recipient.sol/abstract.IERC2771Recipient.md(1 hunks)docs/docs/contracts/src/src/TestingErc20.sol/contract.TestingErc20.md(0 hunks)docs/docs/contracts/src/src/faucet/Erc20Minter.sol/contract.Erc20Minter.md(0 hunks)docs/docs/contracts/src/src/faucet/Erc20Minter.sol/interface.Erc20Mintable.md(0 hunks)docs/docs/contracts/src/src/gateway/CommonStructs.sol/struct.StrategySlippageArgs.md(0 hunks)docs/docs/contracts/src/src/paymasters/AccountAbstraction/AATokenPaymaster.sol/contract.PimlicoERC20Paymaster.md(0 hunks)docs/docs/contracts/src/src/paymasters/AccountAbstraction/SafeTransferLib.sol/library.SafeTransferLib.md(0 hunks)docs/docs/contracts/src/src/paymasters/OnboardingPaymaster.sol/contract.OnboardingPaymaster.md(0 hunks)docs/docs/contracts/src/src/paymasters/Oracle.sol/contract.DummyOracle.md(0 hunks)docs/docs/contracts/src/src/paymasters/Oracle.sol/interface.IOracle.md(0 hunks)docs/docs/contracts/src/src/paymasters/OracleTokenPaymaster.sol/contract.OracleTokenPaymaster.md(0 hunks)docs/docs/contracts/src/src/swap/Btc_Marketplace.sol/contract.BtcMarketPlace.md(1 hunks)docs/docs/contracts/src/src/swap/Faucet.sol/contract.Faucet.md(0 hunks)docs/docs/contracts/src/src/swap/Faucet.sol/interface.Erc20Mintable.md(0 hunks)docs/docs/contracts/src/src/swap/Marketplace.sol/contract.MarketPlace.md(1 hunks)docs/docs/contracts/src/src/swap/Wrapped.sol/contract.BobWrappedBtc.md(0 hunks)
💤 Files with no reviewable changes (13)
- docs/docs/contracts/src/src/gateway/CommonStructs.sol/struct.StrategySlippageArgs.md
- docs/docs/contracts/src/src/paymasters/OnboardingPaymaster.sol/contract.OnboardingPaymaster.md
- docs/docs/contracts/src/src/swap/Wrapped.sol/contract.BobWrappedBtc.md
- docs/docs/contracts/src/src/faucet/Erc20Minter.sol/contract.Erc20Minter.md
- docs/docs/contracts/src/src/paymasters/AccountAbstraction/SafeTransferLib.sol/library.SafeTransferLib.md
- docs/docs/contracts/src/src/TestingErc20.sol/contract.TestingErc20.md
- docs/docs/contracts/src/src/paymasters/Oracle.sol/contract.DummyOracle.md
- docs/docs/contracts/src/src/paymasters/OracleTokenPaymaster.sol/contract.OracleTokenPaymaster.md
- docs/docs/contracts/src/src/swap/Faucet.sol/contract.Faucet.md
- docs/docs/contracts/src/src/paymasters/AccountAbstraction/AATokenPaymaster.sol/contract.PimlicoERC20Paymaster.md
- docs/docs/contracts/src/src/swap/Faucet.sol/interface.Erc20Mintable.md
- docs/docs/contracts/src/src/faucet/Erc20Minter.sol/interface.Erc20Mintable.md
- docs/docs/contracts/src/src/paymasters/Oracle.sol/interface.IOracle.md
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/docs/contracts/src/src/swap/Btc_Marketplace.sol/contract.BtcMarketPlace.md
- docs/docs/contracts/src/src/swap/Marketplace.sol/contract.MarketPlace.md
🧰 Additional context used
🪛 LanguageTool
docs/docs/contracts/src/src/ERC2771/IERC2771Recipient.sol/abstract.IERC2771Recipient.md
[style] ~4-~4: Consider a shorter alternative to avoid wordiness.
Context: ... contract must implement this interface in order to support relayed transaction. It is rec...
(IN_ORDER_TO_PREMIUM)
[uncategorized] ~43-~43: A comma might be missing here.
Context: ... call. For a call that came through the Forwarder the real sender is extracted from the l...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~43-~43: A comma may be missing after the conjunctive/linking adverb ‘Otherwise’.
Context: ...om the last 20 bytes of the msg.data. Otherwise simply returns msg.sender.| ### _ms...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
docs/docs/contracts/src/src/ERC2771/ERC2771Recipient.sol/abstract.ERC2771Recipient.md
[uncategorized] ~26-~26: Possible missing article found.
Context: ...ipient. Only trust verified Forwarder. Method is not a required method to allow Recip...
(AI_HYDRA_LEO_MISSING_THE)
[uncategorized] ~80-~80: A comma might be missing here.
Context: ... call. For a call that came through the Forwarder the real sender is extracted from the l...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~80-~80: A comma may be missing after the conjunctive/linking adverb ‘Otherwise’.
Context: ...om the last 20 bytes of the msg.data. Otherwise simply returns msg.sender.| ### _ms...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
🔇 Additional comments (16)
docs/docs/contracts/src/src/ERC2771/ERC2771Recipient.sol/abstract.ERC2771Recipient.md (9)
1-3: File Header & Git Source Link Looks Good
The header and Git source reference appear appropriate and provide clear context for the document.
4-8: Inherits Section Is Clear
The inheritance section clearly documents the dependency onIERC2771Recipient. No issues found.
14-19: State Variable Documentation Is Clear
The documentation and code block for the_trustedForwarderstate variable are well formatted and informative.
30-32:getTrustedForwarderFunction Prototype Is Correct
The function signature is correctly specified.
33-39: Return Documentation forgetTrustedForwarderIs Clear
The table properly outlines the return value. No changes required.
40-45:_setTrustedForwarderFunction Documentation Is Clear
The description and code block for_setTrustedForwarderare straightforward and require no modifications.
55-67: Parameter and Return Tables forisTrustedForwarderAre Clear
The tables outline both parameters and returns as expected.
73-75:_msgSenderFunction Prototype Is Correct
The function signature for_msgSenderis correctly defined.
83-90:_msgDataFunction Prototype and Context Are Clear
The documentation and code block for the_msgDatafunction are clear and well-structured.docs/docs/contracts/src/src/ERC2771/IERC2771Recipient.sol/abstract.IERC2771Recipient.md (7)
1-3: Header and Git Source Link Verification.The header and Git source link are clear and informative. Please ensure that the linked repository is up-to-date with any recent remappings or changes in the codebase.
6-6: Inheritance Recommendation is Clear.The recommendation to inherit from the ERC2771Recipient contract is clear. You might consider emphasizing the benefits or linking to additional documentation if applicable.
15-17: Function Signature is Well-Documented.The Solidity function signature for
isTrustedForwarderis clearly presented and follows best practices.
36-38: _msgSender Function Signature Validated.The
_msgSenderfunction signature is correct and its accompanying code block is well-presented.
47-49: _msgData Documentation is Clear.The guidance on using
_msgDatainstead ofmsg.datawhen the exact call data matters is straightforward and informative.
51-53: _msgData Function Signature is Correct.The function signature for
_msgDatais clearly defined in the provided code block, meeting the documentation standards.
54-59: _msgData Returns Section is Detailed and Informative.The returns table effectively explains how
_msgDataprocesses the input, especially noting that it strips the last 20 bytes if the call is relayed. The detailed description ensures that developers understand this behavior.
|
Will leave for now since interface is minimal. |
Summary by CodeRabbit
Summary by CodeRabbit