-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
imp: use uint64 for height everywhere #366
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #366 +/- ##
=======================================
Coverage 99.86% 99.86%
=======================================
Files 14 14
Lines 731 731
=======================================
Hits 730 730
Misses 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you quickly measure the gas cost between main and this (you need to remeasure main as well)? I think it shouldn't be much but we should make sure. I was ok with making the change for packets since we assume it is a uint64 when making commitments in the spec.
If this cost too much gas (which I don't think it will). We probably shouldn't merge it as it is not in the IBC specs. The reason why we should measure this is due to decoding costs (not storage)
I've added the gas benchmark changes, but I also added another PR to update main first, so we can see the real difference: #367 |
README.md
Outdated
| `ICS26Router.sol` | `sendPacket` | Initiating an IBC transfer with an `ERC20`. | ~150,000 | ~150,000 | | ||
| `ICS26Router.sol` | `recvPacket` | Receiving _back_ an `ERC20` token. | ~509,542 | ~592,518 | | ||
| `ICS26Router.sol` | `recvPacket` | Receiving a _new_ Cosmos token for the first time. (Deploying an `ERC20` contract) | ~1,079,133 | ~1,163,441 | | ||
| `ICS26Router.sol` | `ackPacket` | Acknowledging an ICS20 packet. | ~393,224 | ~477,022 | | ||
| `ICS26Router.sol` | `timeoutPacket` | Timing out an ICS20 packet | ~465,360 | ~549,821 | | ||
| `ICS26Router.sol` | `sendPacket` | Initiating an IBC transfer with an `ERC20`. | ~165,000 | ~165,000 | | ||
| `ICS26Router.sol` | `recvPacket` | Receiving _back_ an `ERC20` token. | ~518,340 | ~602,020 | | ||
| `ICS26Router.sol` | `recvPacket` | Receiving a _new_ Cosmos token for the first time. (Deploying an `ERC20` contract) | ~1,087,699 | ~1,171,433 | | ||
| `ICS26Router.sol` | `ackPacket` | Acknowledging an ICS20 packet. | ~392,851 | ~476,578 | | ||
| `ICS26Router.sol` | `timeoutPacket` | Timing out an ICS20 packet | ~466,083 | ~550,208 | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You didn't update the aggregated benchmarks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, will fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a benchmark and check for the message sizes? It seems that the encoded message sizes are really important for skip-go to be able to submit txs
Good point. I'll make one for main first so we'll have a diff |
I added this PR for this: https://github.com/cosmos/solidity-ibc-eureka/pull/372/files |
@@ -202,8 +202,8 @@ Since there is no meaningful difference in gas costs between plonk and groth16 i | |||
|
|||
| **ICS26Router Method** | **Description** | **Avg Gas (25 packets)** | **Avg Gas (50 packets)** | **Calldata size (25 packets)** | **Calldata size (50 packets)** | | |||
|:---:|:---:|:---:|:---:|:---:|:---:| | |||
| `multicall/recvPacket` | Receiving _back_ an `ERC20` token. | ~178,272 | ~165,240 | ~51,172B | ~100,772B | | |||
| `multicall/ackPacket` | Acknowledging an ICS20 packet. | ~92,269 | ~86,564 | ~53,572B | ~105,572B | | |||
| `multicall/recvPacket` | Receiving _back_ an `ERC20` token. | ~178,487 | ~172,258 | ~51,172B | ~100,772B | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
avg 50 packet receive increased by 8k?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, seems like I forgot to update that one last time in main :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got 172,275 running it on main
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Description
fixes sherlock issue 176
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.
godoc
comments.Files changed
in the GitHub PR explorer.SonarCloud Report
in the comment section below once CI passes.