feat: update solidity implementation to work with new packet commitment keys #101
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #101 +/- ##
==========================================
+ Coverage 98.59% 98.62% +0.02%
==========================================
Files 12 12
Lines 357 364 +7
Branches 11 11
==========================================
+ Hits 352 359 +7
Misses 5 5 ☔ View full report in Codecov by Sentry. |
| @@ -33,7 +32,7 @@ library ICS24Host { | |||
| pure | |||
| returns (bytes memory) | |||
| { | |||
| return abi.encodePacked("commitments/channels/", channelId, "/sequences/", Strings.toString(sequence)); | |||
| return abi.encodePacked(channelId, uint8(1), uint64ToBigEndian(sequence)); | |||
There was a problem hiding this comment.
Does big endian make any difference in encode packed? If encodePacked is using little endian by default, could we ask ibc-go to use little endian instead?
There was a problem hiding this comment.
We can check if this would be fine with spec @AdityaSripal @sangier, but this is the current ibc-go implementation, so I suggest we do that change in a separate issue in that case.
There was a problem hiding this comment.
its better for it to be bigEndian since it allows us to reverseIterate in order. So ordered trees will have better iterators with this key format
There was a problem hiding this comment.
Does anyone actually iterate these keys in reverse order? Asking because we might be more gas efficient if we don't do this in solidity, and simplifies the implementation.
src/utils/ICS24Host.sol
Outdated
| /// @notice Convert a uint64 to big endian bytes representation | ||
| /// @param value The uint64 value | ||
| /// @return The big endian bytes representation | ||
| function uint64ToBigEndian(uint64 value) internal pure returns (bytes8) { |
There was a problem hiding this comment.
I wanna understand better why we need this function and:
- If we can get rid of this by using little endian in ibc-go or some other change in ibc-go
- If we can find an external library that has this audited function. (Not super needed)
There was a problem hiding this comment.
This is the reasoning I believe: cosmos/ibc-go#7132
- Lets ask speccers about it
- I have not been able to find a library that does this
Co-authored-by: srdtrk <[email protected]>
Co-authored-by: srdtrk <[email protected]>
| @@ -33,7 +32,7 @@ library ICS24Host { | |||
| pure | |||
| returns (bytes memory) | |||
| { | |||
| return abi.encodePacked("commitments/channels/", channelId, "/sequences/", Strings.toString(sequence)); | |||
| return abi.encodePacked(channelId, uint8(1), uint64ToBigEndian(sequence)); | |||
There was a problem hiding this comment.
Does anyone actually iterate these keys in reverse order? Asking because we might be more gas efficient if we don't do this in solidity, and simplifies the implementation.
srdtrk
deleted the
gjermund/98-update-solidity-implementation-to-work-with-new-packet-commitment-keys
branch
Description
Still a couple of PRs left to be merged on the ibc-go side, so still using a separate branch for the simapp image, but that should be done with a simple change with #94.
It looks like no major changes in terms of gas cost, so didn't update the benchmarks this time around.
closes: #98
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.