-
Notifications
You must be signed in to change notification settings - Fork 10
832-seaport-sales-plugin #872
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 659913f The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 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 Git ↗︎
|
e552077
to
58e6ab0
Compare
f386e19
to
bea6350
Compare
bea6350
to
b493620
Compare
b493620
to
6219a6c
Compare
6219a6c
to
8dbc1d6
Compare
8dbc1d6
to
91f1088
Compare
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.
@Zimtente very nice work 🚀 I shared some feedback, feel free to follow up.
1c667ae
to
bdac09a
Compare
bdac09a
to
b3302d6
Compare
b3302d6
to
edfdfb8
Compare
edfdfb8
to
9d6fae4
Compare
…ll index Seaport-Sales across all other name-plugins we support (ENS, 3dns etc)
4c3fab3
to
f8433f1
Compare
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
@@ -217,3 +225,94 @@ export function getChainName(chainId: number): string { | |||
|
|||
return chainName; | |||
} | |||
|
|||
/** | |||
* Returns an array of 0 or more ChainAddress objects that are known to issue tokens |
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 you help me understand the motivation for noting here "... that are compatible with Seaport."?
As I understand, the goal of this function goes beyond anything Seaport-specific. This function is for all contracts known to provide tokenized name ownership within the provided namespace.
* Returns an array of 0 or more ChainAddress objects that are known to issue tokens | |
* Returns an array of 0 or more ChainAddress objects that are known to provide tokenized name ownership. |
Goal: These are not just contracts that issue tokens (ex: any arbitrary token for anything at all). It's contracts that provide tokenized name ownership.
}, | ||
]; | ||
} | ||
case ENSNamespaceIds.Holesky: |
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.
As I understand we should fill this function out as much as we can based on all the contracts we have reference to already in this datasources
package, including for Holesky and the EnsTextEnv.
Please see related comment above. As I understand, this function isn't constrained only to seaport. Our indexing of sales is constrained to seaport. But this function isn't.
const { value: config } = ctx; | ||
|
||
if (config.plugins.includes(PluginName.Seaport)) { | ||
const nameProtocols = [ |
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 you please help me understand the main issue that occurs if only the Seaport plugin is activated?
What does that break? Suggest we add a comment here explaining what the issue is more specifically.
const { value: config } = ctx; | ||
|
||
if (config.plugins.includes(PluginName.Seaport)) { | ||
const nameProtocols = [ |
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.
Not sure about calling these nameProtocols
. This is an array of plugin names. We're putting great care and focus on our terminology.
It will help to better understand the deeper issue (see related comment). Appreciate your suggestion on the optimal name we can give to the ideas in this function.
|
||
// OLD ENS Registry: tokenId is labelhash, need to find domain by labelhash | ||
if (contractAddress === baseRegistrarContractAddress) { | ||
const domains = await context.db.sql.query.domain.findMany({ |
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.
More broadly, I don't understand why we need to perform any DB lookup operation here at all.
If the contractAddress is the baseRegistrarContractAddress then we know the provided tokenId is a labelhash and we also know the parent name is "eth". Therefore we have all the information we need to calculate the node / namehash of the provided tokenId without any need for db operations.
This utility function should be updated such that:
- Any need for db lookup operations is removed.
- The context param should be removed.
- The function should be moved into the datasources package. Nothing about this logic is seaport specific.
const totalAmount = getTotalPaymentAmount(paymentItems); | ||
const currencyAddress = getPaymentTokenAddress(paymentItems); | ||
|
||
const contractAddress = nftItem.token as Address; |
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.
Why "... as Address" here?
@@ -3,5 +3,6 @@ | |||
*/ | |||
export * from "./subgraph.schema"; | |||
export * from "./resolver-records.schema"; | |||
export * from "./seaport.schema"; |
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.
This is not a seaport.schema. It's a tokenscope.schema.
@@ -0,0 +1,32 @@ | |||
import { index, onchainTable } from "ponder"; |
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.
Please update the name of this file.
orderHash: t.hex().notNull(), | ||
tokenId: t.text().notNull(), | ||
contractAddress: t.hex().notNull(), | ||
domainId: t.hex().notNull(), |
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 don't see us creating any foreign key constraints on this field. Therefore why is it needed to enforce any dependency between plugins? Please see other related comments on how we don't need db queries to build domainId.
#832