832-seaport-sales-plugin #872
Conversation
🦋 Changeset detectedLatest commit: a78c927 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Zimtente
force-pushed
the
832-seaport-sales-plugin
branch
from
e552077 to
58e6ab0
Compare
Zimtente
force-pushed
the
832-seaport-sales-plugin
branch
from
f386e19 to
bea6350
Compare
Zimtente
force-pushed
the
832-seaport-sales-plugin
branch
from
bea6350 to
b493620
Compare
Zimtente
force-pushed
the
832-seaport-sales-plugin
branch
from
b493620 to
6219a6c
Compare
Zimtente
force-pushed
the
832-seaport-sales-plugin
branch
from
6219a6c to
8dbc1d6
Compare
Zimtente
force-pushed
the
832-seaport-sales-plugin
branch
from
8dbc1d6 to
91f1088
Compare
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@Zimtente Reviewed and shared feedback.
| import { index, onchainTable } from "ponder"; | ||
|
|
||
| const sharedEventColumns = (t: any) => ({ | ||
| id: t.text().primaryKey(), |
There was a problem hiding this comment.
@Zimtente We need to document ideas like this in our code rather than in GitHub comments.
11155111-5584563-41
What are each of these values? Please document it all in detail. Thanks.
Zimtente
force-pushed
the
832-seaport-sales-plugin
branch
from
659913f to
64609b1
Compare
Zimtente
force-pushed
the
832-seaport-sales-plugin
branch
from
64609b1 to
51ec931
Compare
Zimtente
force-pushed
the
832-seaport-sales-plugin
branch
from
51ec931 to
751754f
Compare
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@Zimtente Thanks for updates. Reviewed and shared feedback
packages/datasources/src/index.ts
Outdated
| contractAddress: Address, | ||
| tokenId: string, | ||
| ): Hex { | ||
| const tokenIdHex = `0x${BigInt(tokenId).toString(16).padStart(64, "0")}` as Hex; |
There was a problem hiding this comment.
Data conversion operations like this should be pushed "to the edge" of our I/O operations. Within our core "business logic" all types should already be "pure".
Therefore, the type of the tokenId param should be Hex and any necessary conversion operation should be moved close to the moment we capture the value through an I/O operation.
For operations like these, also best to:
- Build a named utility function for it. Consider adding it to one of our packages if you think it might be more broadly useful.
- Define unit tests.
Edit: Please also take a look at our uint256ToHex32 function. Perhaps this already does what you want?
There was a problem hiding this comment.
What exactly do you want to be unit tested?
There was a problem hiding this comment.
Any utility function that might be defined here.
packages/datasources/src/index.ts
Outdated
| return makeSubdomainNode(tokenIdHex, ETH_PARENT_NODE); | ||
| } | ||
|
|
||
| // for other names we for now assume it is already namehash |
There was a problem hiding this comment.
This is an important assumption for us to verify and guard against.
This datasources package is publicly published to NPM and aims to be one of the useful packages we're building for the ENS ecosystem. We therefore need to ensure everything inside of it is noobie-proof where possible and that we assume it will be used by others in contexts other than our own personal contexts.
It needs to be generally usable and accurate.
Therefore, a few suggestions:
- For safety,
getDomainIdByTokenIdshould callisKnownTokenIssuingContractand throw a helpful error message if that function returns false. - Update the docs for
getDomainIdByTokenIdto note this possible error case. - For all possible values passed to
isKnownTokenIssuingContractthat might returntrue, can you please manually verify that this assumption of using namehash is correct? - After this manual verification is completed, we should refine this comment since it should not be an assumption, it should be a guarantee that we have already verified. Anytime we expand the set of values that pass
isKnownTokenIssuingContractwe must also verify that the guarantee here holds. - We should add docs to the
isKnownTokenIssuingContractfunction to note that any time we expand the set of known token issuing contracts, we also need to verifygetDomainIdByTokenIdwill continue to always return the correct results.
# Conflicts: # packages/ensnode-sdk/src/ens/constants.ts # packages/ensnode-sdk/src/ens/index.ts # packages/ensnode-sdk/src/ens/types.ts # packages/ensnode-sdk/src/index.ts # pnpm-lock.yaml
# Conflicts: # apps/ensindexer/.env.local.example # packages/datasources/src/index.ts # packages/datasources/src/mainnet.ts # packages/datasources/src/sepolia.ts # pnpm-lock.yaml
|
Superseded by #872 |
#832