Subgraph Logic Fixes Post 21M Diff #38
Conversation
shrugs
requested review from
tk-o,
djstrong,
BanaSeba and
lightwalker-eth
as code owners
src/handlers/NameWrapper.ts
Outdated
| @@ -156,6 +156,9 @@ export const makeNameWrapperHandlers = (ownedName: `${string}eth`) => { | |||
| // NOTE: subgraph does an implicit ignore if no WrappedDomain record. | |||
| // we will be more explicit and update logic if necessary | |||
| await context.db.update(schema.wrappedDomain, { id: node }).set({ fuses }); | |||
|
|
|||
| // materialize the domain's expiryDate | |||
| await materializeDomainExpiryDate(context, node); | |||
| // So here we use the labelhash fn to determine whether ponder modified our `name` argument, | ||
| // in which case we know that it used to have null bytes in it, and we should ignore it. |
There was a problem hiding this comment.
Does that mean we don't at all index names that include null bytes, or we do it somewhere else?
There was a problem hiding this comment.
My understanding is that we will still index names with labels that include null bytes, but the way we index them will to represent these labels as "unknown labels", rather than as label literals.
@shrugs Could you please confirm? Thanks
There was a problem hiding this comment.
it means that we ignore events that have names with null bytes
There was a problem hiding this comment.
Noted. I've created an issue to track an enhancement opportunity: #45
| offset += len; | ||
| len = buf[offset++]; | ||
| } | ||
| return [firstLabel, list.join("")]; |
There was a problem hiding this comment.
@lightwalker-eth this one looks like something you'd be the best person to review :) I can only suggest creating unit tests for. But I'm also aware I have to first integrate a testing suite into this project as per
There was a problem hiding this comment.
@tk-o Agreed 👍 I shared a comment for us to follow up on this opportunity in a separate future PR here: #15 (comment)
There was a problem hiding this comment.
we're copying and pasting subgraph code, testing the function is lower priority than adding tests to ensjs for this case, imo
There was a problem hiding this comment.
This is definitely a lower priority goal at the moment, but as I understand there's still value for us to define our own unit tests for decodeDNSPacketBytes under the assumption that ensjs defines bytesToPacket but not decodeDNSPacketBytes.
If my assumptions are off there please let me know. Cheers 👍
There was a problem hiding this comment.
those are intended to be the same function, although we do need to inject the subgraph-specific character checks (isLabelIndexable) into their logic as well (their util should probably handle that case tbh). but yeah some tests here are ok, i was cautioning against trying to exhaustively understand and test this function when its guaranteed to do what we desire for the moment (because it comes from subgraph and we're aiming for subgraph 1:1)
There was a problem hiding this comment.
@shrugs Looks good 🚀 Shared 1 comment seeking your confirmation.
| offset += len; | ||
| len = buf[offset++]; | ||
| } | ||
| return [firstLabel, list.join("")]; |
There was a problem hiding this comment.
@tk-o Agreed 👍 I shared a comment for us to follow up on this opportunity in a separate future PR here: #15 (comment)
| // So here we use the labelhash fn to determine whether ponder modified our `name` argument, | ||
| // in which case we know that it used to have null bytes in it, and we should ignore it. |
There was a problem hiding this comment.
My understanding is that we will still index names with labels that include null bytes, but the way we index them will to represent these labels as "unknown labels", rather than as label literals.
@shrugs Could you please confirm? Thanks
| * TODO: replace this function with ensjs#bytesToPacket when it correctly handles these cases. See | ||
| * commit hash bace0ab55077d9f5cd37bd9d6638c4acb16334a8 for an example implementation. | ||
| * | ||
| */ | ||
| export function decodeDNSPacketBytes(buf: Uint8Array): [string | null, string | null] { |
There was a problem hiding this comment.
Noted a follow up opportunity that we can action in a separate future PR: #41 (comment)
|
subgraph excludes events that contain null byte names, we are doing the same. it specifically only excludes them when setting 'preimage' (whatever that is) for the name registered by controller and name renewed by controller events. this implementation does the same. only addition here is correctly identifying when ponder has already removed the null bytes that we were trying to detect with isLabelIndexable |
| // So here we use the labelhash fn to determine whether ponder modified our `name` argument, | ||
| // in which case we know that it used to have null bytes in it, and we should ignore it. |
There was a problem hiding this comment.
Noted. I've created an issue to track an enhancement opportunity: #45
| offset += len; | ||
| len = buf[offset++]; | ||
| } | ||
| return [firstLabel, list.join("")]; |
There was a problem hiding this comment.
This is definitely a lower priority goal at the moment, but as I understand there's still value for us to define our own unit tests for decodeDNSPacketBytes under the assumption that ensjs defines bytesToPacket but not decodeDNSPacketBytes.
If my assumptions are off there please let me know. Cheers 👍
closes #36