-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
fix: Use fixed length hex for pointer at FwdCapabilityKey #11737
fix: Use fixed length hex for pointer at FwdCapabilityKey #11737
Conversation
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! We have discussion in the issue for better longer term solutions (one of which seems like it may be simple if we understand correctly), but I see no reason to block this which is clearly correct and solves the immediate bug
Codecov Report
@@ Coverage Diff @@
## main #11737 +/- ##
==========================================
+ Coverage 66.00% 66.02% +0.02%
==========================================
Files 668 668
Lines 70695 70695
==========================================
+ Hits 46659 46676 +17
+ Misses 21345 21328 -17
Partials 2691 2691
|
@AdityaSripal should this be backported to current releases (0.44.x, 0.45.x, 0.46.x)? This isn't state breaking AFAIk, as nodes can just restart safely. |
@alexanderbez given that this changes the amount of gas used, and that gets used to calculate a hash that gets stored, maybe it is state breaking? |
Right...I forgot that happened, which is unfortunate. Why we charge gas for memory-based operations is questionable. |
…osmos#11737) (cosmos#12818) * fix: Use fixed length hex for pointer at FwdCapabilityKey (backport cosmos#11737) * Update CHANGELOG.md * add comments and unit tests
Description
Use
%#016p
instead of%p
to standardize across architectures. This will pad pointer addresses up to 16 chars (enough to do MaxUint64) and will remove the leading 0x in an effort to reduce the gas usage impact (so in the end we are only adding 3 bytes instead of 5).Closes: #11726
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking change <- This IS a breaking change right?CHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking changecc @marbar3778 @alexanderbez @ValarDragon