-
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
feat(store): add splice events #1354
Conversation
🦋 Changeset detectedLatest commit: 9a2382c The changes in this PR will be included in the next version bump. This PR includes changesets to release 28 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
StoreSpliceField
eventStoreSpliceRecord
event
data: log.args.data, | ||
start: log.args.start, | ||
deleteCount: log.args.deleteCount, |
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.
Is there a way we could perform the steps to combine the new bytes with an existing record in here? Or would that prevent some optimizations we can do downstream (like encode+splice+decode in a single db tx)?
packages/store/gas-report.json
Outdated
@@ -693,7 +693,7 @@ | |||
"file": "test/StoreCoreGas.t.sol", | |||
"test": "testSetAndGetField", | |||
"name": "set static field (1 slot)", | |||
"gasUsed": 33280 | |||
"gasUsed": 33581 |
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.
+300 in gas; some increase is expected, is there anything we can do to further optimize this or is this as good as it gets?
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 didn't intentionally spend time optimizing gas here, probably can do better when I get to StoreCore
packages/store/src/StoreCore.sol
Outdated
// Prepare data for the splice event | ||
uint256 start; | ||
unchecked { | ||
// (safe because it's a few uint40 values, which can't overflow uint48) | ||
start = valueSchema.staticDataLength() + 32; | ||
for (uint8 i; i < dynamicSchemaIndex; i++) { | ||
start += encodedLengths.atIndex(i); | ||
} | ||
} | ||
uint256 deleteCount = oldFieldLength; | ||
// Emit event to notify indexers | ||
emit StoreCore.StoreSpliceRecord(tableId, key, uint48(start), uint40(deleteCount), data); |
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.
Is it worth generalizing this in an internal util function, since very similar logic is used for _setDynamicField
, _pushToDynamicField
, _popFromDynamicField
and _setDynamicFieldItem
? I feel like might declutter the code a bit, make it easier to audit and easier to write tests for this logic in isolation
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've been thinking about this for a while now. I think the only efficient way to declutter is generalize it in a renderer function, and render StoreCore. Using solidity functions means more gas because the optimizer won't inline any of it and we'll need to return many vars
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.
Or we could not worry about gas I guess, it's probably less than 1000 (haven't tested specifics). Though a bunch of return vars may still be ugly compared to a renderer with conditionals in the relevant places
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.
Unfortunately I think a renderer would make auditing harder (because auditors have less experience with codegen). I'd be curious about the gas implications of moving it to a helper function, if it's a significant increase i'm also fine with keeping it inline
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.
@alvrs do you want declutter in this PR? it feels like it'd be better in a separate one for easier reverts and gas comparisons
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 try to refactor in a separate PR
packages/store/src/PackedCounter.sol
Outdated
// We use a specific, invalid packed counter (total length != sum of lengths) to represent "unchanged" for the purpose of events and off-chain indexing | ||
bytes32 constant UNCHANGED_PACKED_COUNTER = bytes32(uint256(1)); | ||
|
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 still need this? doesn't seem like it's used
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.
we're not using it yet so feel free to trash (which I see you've already done), can add later
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 follow up issue for splice tests: #1515 |
Co-authored-by: Kevin Ingersoll <[email protected]> Co-authored-by: alvrs <[email protected]>
fixes #1222
fixes #1296
TODO:
bytes16
fromSchema.staticDataLength()
? since that's what it's actually stored as?figure out if its gas efficient enough to remove total length from encoded lengthsmoved to explore removing total length from PackedCounter #1420