feat: v2 contracts natspec #249
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe recent updates enhance the clarity, documentation, and functionality of multiple Solidity contracts, focusing on improved error handling, detailed NatSpec comments, and new event definitions. These changes support better developer understanding and interaction with the contracts, particularly for cross-chain operations and token management. The introduction of new access control mechanisms and refined documentation ensures robust contract interactions while preserving core logic. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Gateway
participant TSS
participant Contract
User->>TSS: Initiates transaction
TSS->>Gateway: Calls execute method
Gateway->>Contract: Executes cross-chain call
Contract-->>Gateway: Returns result
Gateway-->>TSS: Returns response
TSS-->>User: Notifies user of outcome
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| function setConnector(address _zetaConnector) external { | ||
| /// @notice Sets the connector contract address. | ||
| /// @param _zetaConnector Address of the connector contract. | ||
| function setConnector(address _zetaConnector) external { |
Check warning
Code scanning / Slither
Conformance to Solidity naming conventions Warning
| /// @dev This function can only be called by the TSS address and it is payable. | ||
| /// @param destination Address to call. | ||
| /// @param data Calldata to pass to the call. | ||
| function executeRevert(address destination, bytes calldata data) public payable onlyTSS { |
Check notice
Code scanning / Slither
Missing zero address validation Low
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #249 +/- ##
==========================================
- Coverage 61.40% 60.29% -1.11%
==========================================
Files 16 16
Lines 399 403 +4
Branches 106 108 +2
==========================================
- Hits 245 243 -2
- Misses 154 160 +6 ☔ View full report in Codecov by Sentry. |
| // As the current version, ERC20CustodyNew hold the ERC20s deposited on ZetaChain | ||
| // This version include a functionality allowing to call a contract | ||
| // ERC20Custody doesn't call smart contract directly, it passes through the Gateway contract | ||
| /// @title ERC20CustodyNew |
There was a problem hiding this comment.
nit: are we going to keep the "New" suffixes in the contracts? Should we omit then as this contracts move to production?
| */ | ||
| /// @title GatewayEVM | ||
| /// @notice The GatewayEVM contract is the endpoint to call smart contracts on external chains. | ||
| /// @dev The contract doesn't hold any funds and should never have active allowances. |
There was a problem hiding this comment.
nit: Should we open an issue to implement this logic specifically?
fallback() external payable {
revert();
}
receive() external payable {
revert();
}
There was a problem hiding this comment.
Is receive for ERC777?
I think makes sense to add the fallback, at the end I don't think we can prevent sending ERC20, the assumption above is more that it can't receive and hold funds from the TSS or ERC20Custody
There was a problem hiding this comment.
lets open issue, i also think we would need check if sender is tss and dont revert in that case? but receiving eth/erc20 cant cause issues right?
There was a problem hiding this comment.
LGTM
@coderabbitai can you please review and check if there are discrepancies in the natspecs compared to functions?
| */ | ||
| /// @title GatewayEVM | ||
| /// @notice The GatewayEVM contract is the endpoint to call smart contracts on external chains. | ||
| /// @dev The contract doesn't hold any funds and should never have active allowances. |
There was a problem hiding this comment.
Is receive for ERC777?
I think makes sense to add the fallback, at the end I don't think we can prevent sending ERC20, the assumption above is more that it can't receive and hold funds from the TSS or ERC20Custody
| function setConnector(address _zetaConnector) external onlyTSS { | ||
| /// @notice Sets the connector contract address. | ||
| /// @param _zetaConnector Address of the connector contract. | ||
| function setConnector(address _zetaConnector) external onlyTSS { |
Check warning
Code scanning / Slither
Conformance to Solidity naming conventions Warning
skosito
requested review from
andresaiello,
brewmaster012,
CharlieMc0 and
fadeev
as code owners
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores