-
Notifications
You must be signed in to change notification settings - Fork 196
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(protocol-parser): only append packed counter if there is dynamic data #1097
Conversation
alvrs
commented
Jul 3, 2023
•
edited
Loading
edited
- The packed counter is only appended to the encoded data if there is dynamic data: https://github.com/latticexyz/mud/blob/main/packages/store/src/StoreCore.sol#L434-L442
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
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.
do we want a test for this?
good call, that would be good, also as a baseline for folks who want to port this |
fcac033
to
54a19b2
Compare
00c0175
to
2f9ad25
Compare
? `${encodeField("uint56", dynamicTotalByteLength).replace(/^0x/, "")}${dynamicFieldByteLengths | ||
.map((length) => encodeField("uint40", length).replace(/^0x/, "")) | ||
.join("")}`.padEnd(64, "0") | ||
: ""; |
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.
sorry, one more thing
it might make sense to do this check and return early so we avoid all the other work above and inner conditionals and such
shortly after const staticData = ...
, do something like
if (!dynamicValues.length) {
return `0x${staticData}`;
}
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.
agree, this also mirrors the on-chain logic better
cb89f43
to
a425d2d
Compare