-
Notifications
You must be signed in to change notification settings - Fork 1
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
Minting contract #22
Minting contract #22
Conversation
The patch branch is based off the previous commit with Plutonomicon/cardano-transaction-lib@f5785cd applied The use of `/\` in an instance declaration was causing an invalid string literal in the output JS, something like "toMetadataArray/\"
…purchase-metadata
…l/seabug-contracts into calum/purchase-metadata
This branch has the Plutonomicon/cardano-transaction-lib#696 commit merged into it + includes my change to fix the build
…l/seabug-contracts into calum/purchase-metadata
…skell/seabug-contracts into calum/minting-contract
Deleted Token because MintingPolicy is a newer version the same thing
Collection nfts minted using CTL have a few differences
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.
Made issues for the non-critical TODO items
src/Seabug/Types.purs
Outdated
@@ -51,8 +65,7 @@ newtype MintParams = MintParams | |||
price :: Natural | |||
, lockLockup :: BigInt | |||
, lockLockupEnd :: Slot | |||
, fakeAuthor :: Maybe PaymentPubKeyHash | |||
, feeVaultKeys :: Array PubKeyHash -- `List` is also an option | |||
, feeVaultKeys :: Array PubKeyHash -- TODO: check if this 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.
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, to derive hash of fee vault script
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'm guessing that's Dao.hs? And we won't be using this for a while?
-- I put too low. The old nfts are caught above because their | ||
-- metadata won't be parsed. | ||
guard $ (unwrap metadata.seabugMetadata # _.ownerPrice) >= | ||
(Natural.fromInt' (10 * 1000000)) |
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 contract fails if the price is too low, 10 ada was the amount I arrived at as a "minimum". I made some other NFTs lower than this so they need to be filtered 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.
do we know why this fails?
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 price is lower than min ADA you can't really pay, so 2 ADA should work
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'll just update this to use our minAdaOnlyUTxOValue
then
- Uncomment [this line](https://github.com/mlabs-haskell/seabug-contracts/blob/cda88824f87e0b961b738c66a428b7ade77454be/index.js#L39) | ||
- Update the image info [here](https://github.com/mlabs-haskell/seabug-contracts/blob/cda88824f87e0b961b738c66a428b7ade77454be/src/Seabug/Seabug.purs#L34) |
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 using branch specific links here, we'll want to change this to main when we merge up
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.
These are permalinks so they should continue to work (I think). The lines will probably get moved around, but this seems like a good way to do this since it should be easy to figure out what lines need to be acted on by comparing the old linked version with the updated version
Seems likely to break things when used across multiple branches
@@ -68,6 +68,6 @@ marketPlaceListNft = do | |||
-- I put too low. The old nfts are caught above because their | |||
-- metadata won't be parsed. | |||
guard $ (unwrap metadata.seabugMetadata # _.ownerPrice) >= | |||
(Natural.fromInt' (10 * 1000000)) | |||
(Natural.fromBigInt' minAdaOnlyUTxOValue) |
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 we verify a 2ada nft can still be bought?
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.
Confirmed that a minAdaOnlyUTxOValue
(actually 1 ada) priced nft can be bought, this is the purchase transaction: https://testnet.cardanoscan.io/transaction/dace69f58dc611579bd5c7a7cf0de1d4b307f11f9fce01663bc7799cc6fd799c
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.
neato!
networkId <- getNetworkId | ||
addr <- liftContractM "Cannot get user address" $ | ||
payPubKeyHashEnterpriseAddress networkId owner | ||
payPubKeyHashBaseAddress networkId owner ownerStake |
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.
Verify this won't be a breaking change, I believe the onchain is coded such that staking addresses aren't permitted right now. We'll need to update that to merge this if this breaks, or move this out to another PR
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.
Minting the token purchased in the above transaction was done with this change, so it seems okay?
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.
Huh, the source code really looked like it wouldn't allow this, well good to know
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.
Staking (enterprise) addresses aren't allowed when changing owner iirc, should work fine for minting. Payment for changing owner has to be sent to address without staking key though.
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.
How did the 1 ada cost buy transaction pass then? Was the ownerstake perhaps still Nothing?
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.
@samuelWilliams99 I think because the buy contract doesn't use a staking key, it uses the ownPaymentPubKeyHash
of the buyer
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.
oh of course! Sorry thats my bad - I thought this change was in the buy contract, but i see its for minting 😅
Okay, Im happy with this then, lets merge :D
Verified fetching a single nft still works after merge |
Closes #21. Also closes #8 and purchase metadata is tested and working with this branch. All changes from #10 are included here because they were used for minting. Merging into calum/purchase-metadata to make it clearer what changes were for metadata and what was for minting.
This PR also includes changes to the parsing of the metadata received from blockfrost, because the nfts minted via CTL have a different metadata format. So the previously minted nfts still on the marketplace won't work after this PR.