fix: refactor and document tokenId decoding logic #94
Conversation
There was a problem hiding this comment.
@shrugs Hey great to see this PR 😄 Reviewed and shared feedback 👍
shrugs
force-pushed
the
fix/label-encoding
branch
from
28c7a07 to
04aed9b
Compare
There was a problem hiding this comment.
@shrugs Nice! Thank you! This is definitely moving in the right direction now. Reviewed and shared feedback 👍
| // NOTE: ponder intentionally removes null bytes to spare users the footgun of | ||
| // inserting null bytes into postgres. We don't like this behavior, though, because it's | ||
| // important that 'vitalik\x00'.eth and vitalik.eth are differentiable. | ||
| // https://github.com/ponder-sh/ponder/issues/1456 | ||
| // 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. | ||
| const didHaveNullBytes = labelhash(name) !== label; | ||
| const didHaveNullBytes = _labelhash(name) !== labelhash; |
There was a problem hiding this comment.
Hmm.. It's not right that we would ever calculate the labelhash of a name.
Is name actually label? Are we able to rename that param? Also appreciate if we could verify from related smart contract logic that this renaming would be appropriate.
Additionally, I just noticed a danger point for us that we need to give very careful consideration to. We're currently using the labelhash implementation in viem that is "unknown label aware": https://github.com/wevm/viem/blob/fe558fdef7e2e9cd5f3f57d8bdeae0c7ff67a1b0/src/utils/ens/labelhash.ts#L20-L33
Therefore, if the string we pass into viem's labelhash is in the format of "[{labelhash}]" where {labelhash} is X, the value returned will be X rather than the keccak256 hash of the label literal, which is the traditional definition of labelhash.
It should also be noted how a similar danger point exists in the viem implementation of namehash. For more details, please see the feedback I just shared on PR 89.
Suggest we review all our calls to labelhash / namehash and see if we need to change something.
Please feel super welcome to ask questions 👍 Happy to help!
There was a problem hiding this comment.
in this context the events (NameRegistered, NameRenewed) emit what we now call a 'label' called name and what we're calling a 'labelhash' called label. the variable names here stem from those events and the subgraph, which names them the same thing.
i've separated these concerns into #100
| @@ -47,35 +48,34 @@ export const makeRegistrarHandlers = (ownedName: OwnedName) => { | |||
|
|
|||
| await context.db | |||
| .update(schema.registration, { | |||
| id: makeRegistrationId(ownedName, label, node), | |||
| id: makeRegistrationId(ownedName, labelhash, node), | |||
| }) | |||
| .set({ labelName: name, cost }); | |||
| } | |||
|
|
|||
| return { | |||
| get ownedSubnameNode() { | |||
There was a problem hiding this comment.
Should we rename this function to ownedNameNode too?
| @@ -51,7 +96,7 @@ export default function () { | |||
|
|
|||
| await handleNameTransferred({ | |||
| context, | |||
| event: { ...event, args: { from, to, tokenId } }, | |||
| event: { ...event, args: { from, to, labelhash: labelhash } }, | |||
There was a problem hiding this comment.
| event: { ...event, args: { from, to, labelhash: labelhash } }, | |
| event: { ...event, args: { from, to, labelhash } }, |
closes #44