-
Notifications
You must be signed in to change notification settings - Fork 80
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
Add protocol sdk and minting features #322
Conversation
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
royaltyRecipient: Address; | ||
}; | ||
|
||
export function create1155TokenSetupArgs({ |
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 more around creating an 1155 using the fixed priced minter. do we want to have it's name to reflect that? cause this naming implies that this is the only way to create an 1155 token
abi: zoraCreator1155FactoryImplABI, | ||
// Since this address is deterministic we can hardcode a chain id safely here. | ||
address: zoraCreator1155FactoryImplAddress[999], | ||
functionName: "deterministicContractAddress", |
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 function only works for contracts created via determinstic contract addresses, i.e. premint.
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 only using this for when people don't pass in a contract address. I'll document it.
} | ||
}; | ||
|
||
async function getContractExists( |
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 think it may be cleaner to just break this into two functions:
const getDeterminsticContractExists({contractConfig})
const getContractExists({address})
}, | ||
}; | ||
|
||
export abstract class ClientBase { |
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.
id avoid having this base class just for holding the chain and network config. how about just having the subclasses contain these objects and have utility functions to extract whats needed from both objects?
inheritance can be kind of hard to follow, and id be concerned that more stuff gets thrown in here than is needed.
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.
fine resolving this in a future PR (to get this out)
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.
The main reason I have things setup this way is the ease to override a network client without adding a ton of parameters.
5e4b6fa
to
a22b027
Compare
4e94bda
to
fe8b4e4
Compare
* Moved premint sdk to protocol sdk * add protocol sdk and minting features --------- Co-authored-by: Dan Oved <[email protected]>
* Moved premint sdk to protocol sdk * add protocol sdk and minting features --------- Co-authored-by: Dan Oved <[email protected]>
* added back build-js task * Add protocol sdk and minting features (#322) * Moved premint sdk to protocol sdk * add protocol sdk and minting features --------- Co-authored-by: Dan Oved <[email protected]> --------- Co-authored-by: Iain Nash <[email protected]>
* added back build-js task * Add protocol sdk and minting features (#322) * Moved premint sdk to protocol sdk * add protocol sdk and minting features --------- Co-authored-by: Dan Oved <[email protected]> --------- Co-authored-by: Iain Nash <[email protected]>
* added back build-js task * Add protocol sdk and minting features (#322) * Moved premint sdk to protocol sdk * add protocol sdk and minting features --------- Co-authored-by: Dan Oved <[email protected]> --------- Co-authored-by: Iain Nash <[email protected]>
* added back build-js task * Add protocol sdk and minting features (#322) * Moved premint sdk to protocol sdk * add protocol sdk and minting features --------- Co-authored-by: Dan Oved <[email protected]> --------- Co-authored-by: Iain Nash <[email protected]>
Add new functions to the
premint-sdk
and rename it toprotocol-sdk
.Mint token (not premint):
(Ideally this can also handle premints but the data paths are too different for now).
Create token/contract: