refactor: cleanup for ICS20 and its tests #253
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #253 +/- ##
==========================================
- Coverage 99.47% 99.46% -0.02%
==========================================
Files 12 12
Lines 572 560 -12
==========================================
- Hits 569 557 -12
Misses 3 3 ☔ View full report in Codecov by Sentry. |
contracts/utils/ICS20Lib.sol
Outdated
| /// @notice removeHop removes the first hop from the denom trace. | ||
| /// @param denom Denom to remove the hop from | ||
| /// @param hop Hop to remove (it must be the first hop) | ||
| /// @return The new denom with the first hop removed | ||
| function removeHop(bytes memory denom, bytes memory hop) internal pure returns (bytes memory) { | ||
| return Bytes.slice(denom, hop.length); | ||
| } |
There was a problem hiding this comment.
I wonder what the efficiency changes would be if you inlined this code instead. We may wanna consider it if it has substantial efficiency. Since function code is also easy to read, you could simply inline this and add a comment inline.
contracts/utils/ICS20Lib.sol
Outdated
| /// @notice addHop adds a hop to the denom trace as the first hop. | ||
| /// @param denom Denom to add the hop to | ||
| /// @param hop Hop to add | ||
| /// @return The new denom with the hop added | ||
| function addHop(bytes memory denom, bytes memory hop) internal pure returns (bytes memory) { | ||
| return abi.encodePacked(hop, denom); | ||
| } |
There was a problem hiding this comment.
ditto about efficiency and the need for this function. The internal logic is easy to read.
There was a problem hiding this comment.
I removed them, but I think maybe the compiler inlines them anyway, because there was 0 gas difference in any tests.
contracts/ICS20Transfer.sol
Outdated
| /// @param ics26Router The ICS26Router contract | ||
| /// @custom:storage-location erc7201:ibc.storage.ICS20Transfer | ||
| struct ICS20TransferStorage { | ||
| IEscrow escrow; | ||
| mapping(string => IBCERC20) ibcDenomContracts; | ||
| mapping(bytes32 => IBCERC20) ibcDenomContracts; |
There was a problem hiding this comment.
| mapping(bytes32 => IBCERC20) ibcDenomContracts; | |
| mapping(bytes32 => IBCERC20) denomIDContracts; |
You forgot to change the name? (Since natspec is changed) but also I don't really like the name too much.
contracts/ICS20Transfer.sol
Outdated
|
|
||
| if (packetData.amount == 0) { | ||
| return ICS20Lib.errorAck("invalid amount: 0"); | ||
| } | ||
|
|
||
| (address receiver, bool receiverConvertSuccess) = ICS20Lib.hexStringToAddress(packetData.receiver); | ||
| (bool receiverConvertSuccess, address receiver) = Strings.tryParseAddress(packetData.receiver); |
There was a problem hiding this comment.
| (bool receiverConvertSuccess, address receiver) = Strings.tryParseAddress(packetData.receiver); | |
| (bool isAddress, address receiver) = Strings.tryParseAddress(packetData.receiver); |
Cleaner name
contracts/ICS20Transfer.sol
Outdated
| IBCERC20(erc20Address).mint(packetData.amount); | ||
| } | ||
|
|
||
| // transfer the tokens to the receiver | ||
| // solhint-disable-next-line multiple-sends |
There was a problem hiding this comment.
| // solhint-disable-next-line multiple-sends |
Why is this needed?
contracts/ICS20Transfer.sol
Outdated
| IBCERC20(erc20Address).mint(packetData.amount); | ||
| } | ||
|
|
||
| // solhint-disable-next-line multiple-sends |
There was a problem hiding this comment.
| // solhint-disable-next-line multiple-sends |
Plz remove
There was a problem hiding this comment.
Lgtm. Might be worth looking into that 0.01% cov.
srdtrk
changed the title
Description
closes: #XXXX
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.