-
Notifications
You must be signed in to change notification settings - Fork 17
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
feat(launchpad): add contract and deps #1315
base: main
Are you sure you want to change the base?
Conversation
WaDadidou
commented
Oct 18, 2024
•
edited
Loading
edited
- Add workspace and root Cargo.toml
- Add a network feature
- Add nft-launchpad contract
- Add nft-tr721 contract
- Add scripts to optimize, deploy and instantiate nft-launchpad contract, and optimize and deploy nft-tr721 contract
✅ Deploy Preview for teritori-dapp ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for testitori ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
4389e54
to
975e22d
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.
partial review
#[cw_serde] | ||
pub struct Config { | ||
pub name: String, | ||
pub nft_code_id: Option<u64>, |
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 make it an Option
?
pub nft_code_id: Option<u64>, | |
pub nft_code_id: u64, |
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 can use nft-launchpad without nft-tr721. Only deploy_collection will not work.
Same for admin, we don't need the admin to use submit_collection or update_merkle_root
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.
when do we want only parts of the functions working?
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 understand.
We don't need tr721 or authority to send CollectionProject data onchain.
The users can create and complete collections.
The admin can deploy
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.
yes okay, but do we really want to allow to instantiate the launchpad with no underlying cw721 implem or no admin?
when does it make sense? do you have a usecase?
I think this just adds uneeded complexity and corner cases
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.
For me, if we don't need it to execute things, it should be not mandatory (dummy style)
But it's not a "smartcontract vision".
I can counter my own argument with this: https://github.com/TERITORI/teritori-dapp/blob/feat-nft-launchpad-contract/rust/cw-contracts/cw721-membership/src/contract.rs#L90
All Config
fields are mandatory, but not necessary used in all the functions of this contract.
I have no use case, I'll make them mandatory ^^
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.
|
||
#[cw_serde] | ||
#[derive(Default)] | ||
pub struct Collection { |
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.
pub struct Collection { | |
pub struct CollectionProject { |
to disambiguate with the actual Collection
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 not just LaunchpadCollection
?
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.
because we're already in the nft-launchpad
context
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.
Remove unused supported_networks Set ConfigChanges all fields optional Rename struct Collection Remove unused CollectionState Remove comments