feat(ics20): use ibc denom #33
Conversation
| string memory ibcDenom_, | ||
| string memory baseDenom_, | ||
| string memory fullDenomPath_ |
There was a problem hiding this comment.
Maybe we should do a validation that these actually match with each other in the constructor?
There was a problem hiding this comment.
I would want to, but would require string parsing which is quite gas costly, so not 100% sure if it is worth the extra cost?
| /// @notice toHexHash converts a string to an all uppercase hex hash (without the 0x prefix) | ||
| /// @param str string to convert | ||
| /// @return uppercase hex hash without 0x prefix | ||
| function toHexHash(string memory str) public pure returns (string memory) { |
There was a problem hiding this comment.
Is this really the hex hash? The output string doesn't seem bounded in length. I thought I'd see a keccak or sha hash somewhere. If we need to do something even more complex, I'm happy to not have the same exact implementation
There was a problem hiding this comment.
In this line:
bytes memory hexBz = bytes(Strings.toHexString(uint256(sha256(bytes(str)))));It does a sha256 of the bytes and Strings.toHexString which is bounded in length. The work it does afterwards is to remove the 0x prefix and uppercase the string.
I used sha256 and did the uppercase conversion to match the ibc-go output of toIBCDenom()
I will comment and clean up the code a bit to make this clearer.
There was a problem hiding this comment.
It looks like sha256 is the faster option anyway, so might as well stick with that: ethereum/consensus-specs#612
I've also cleaned up the code to make it a bit more clear what is happening :)
| function test_toIBCDenom() public pure { | ||
| // Test cases against the ibc-go implementations output | ||
| IBCDenomTestCase[3] memory testCases = [ | ||
| IBCDenomTestCase( | ||
| "transfer/channel-0/uatom", "ibc/27394FB092D2ECCD56123C74F36E4C1F926001CEADA9CA97EA622B25F41E5EB2" | ||
| ), | ||
| IBCDenomTestCase( | ||
| "transfer/channel-0/transfer/channel-52/uatom", | ||
| "ibc/869A01B76A1E87154A4253B3493A2CDD106F4AE6E8F4800C252006C13B20C226" | ||
| ), | ||
| IBCDenomTestCase( | ||
| "transfer/07-tendermint-0/stake", "ibc/D33713CAB4FB7F6E46CDB160183F558E99AFDA3C3A0F22B358273D374BECAA18" | ||
| ) | ||
| ]; | ||
|
|
||
| for (uint256 i = 0; i < testCases.length; i++) { | ||
| IBCDenomTestCase memory testCase = testCases[i]; | ||
| string memory actual = ICS20Lib.toIBCDenom(testCase.denom); | ||
| assertEq(actual, testCase.expected); | ||
| } | ||
| } | ||
| } |
src/ICS20Transfer.sol
Outdated
| @@ -186,10 +195,11 @@ contract ICS20Transfer is IIBCApp, IICS20Transfer, IICS20Errors, Ownable, Reentr | |||
| returns (ICS20Lib.UnwrappedPacketData memory) | |||
| { | |||
| ICS20Lib.PacketDataJSON memory packetData = ICS20Lib.unmarshalJSON(packet.data); | |||
| ICS20Lib.UnwrappedPacketData memory receivePacketData = ICS20Lib.UnwrappedPacketData({ | |||
| denom: packetData.denom, | |||
| ICS20Lib.UnwrappedPacketData memory sendPacketData = ICS20Lib.UnwrappedPacketData({ | |||
There was a problem hiding this comment.
I'm super confused about these functions and find it hard to read. I'm curious if there could be a large refactor in a follow up to simplify this stuff. Even the name UnwrappedPacketData is confusing imo
There was a problem hiding this comment.
Yeah, I would love that too :/
There was a problem hiding this comment.
Made an issue for it: #36
src/utils/ICS20Lib.sol
Outdated
| /// @param originatorChainIsSource True if origniating chain is source of token | ||
| /// @param erc20Contract The address of the ERC20 contract | ||
| /// @param erc20Address The address of the ERC20 contract | ||
| /// @param amount The amount of tokens | ||
| /// @param sender The sender of the tokens | ||
| /// @param receiver The receiver of the tokens | ||
| /// @param memo Optional memo | ||
| struct UnwrappedPacketData { |
There was a problem hiding this comment.
This struct keeps getting more complicated and it already has a weird name. I think we might need to think of another way to pass information (instead of adding new fields to this struct).
There was a problem hiding this comment.
Yeah, I mentioned this struct in the refactor issue: #36
| @@ -99,7 +109,8 @@ contract ICS20Transfer is IIBCApp, IICS20Transfer, IICS20Errors, Ownable, Reentr | |||
| return ICS20Lib.errorAck(abi.encodePacked("unexpected version: ", msg_.packet.version)); | |||
| } | |||
|
|
|||
| ICS20Lib.UnwrappedPacketData memory packetData = _unwrapReceivePacketData(msg_.packet); | |||
| ICS20Lib.PacketDataJSON memory packetData = ICS20Lib.unmarshalJSON(msg_.packet.data); | |||
| (address erc20Address, bool originatorChainIsSource) = getReceiveERC20AddressAndSource(msg_.packet, packetData); | |||
Just for reference: the string parsing work I did is pushed to this branch: https://github.com/srdtrk/solidity-ibc-eureka/tree/gjermund/denom-and-hops-wip