Skip to content

Conversation

@rynoV
Copy link
Contributor

@rynoV rynoV commented Jul 28, 2022

Closes #37 and #30

Changes:

TODO:

  • Move away from extraConfig
    • This feature of CTL is going to be removed eventually, I think I may as well refactor while I'm doing this migration
    • I tried a custom monad stack here, it's not much better than just passing an extra parameter to the contracts that need the projectId
      • We could make it nicer by implementing our own liftX functions to make interoperating with all of CTL's Contract functions easier, but I don't think it's worth it right now
  • Make sure to properly reuse contract environment (websocket connections, etc.)
    • This PR is on par with master in terms of the number of websocket connections
    • It might be possible to refactor so that the contract environment is reused across contract calls, but I think that's premature optimization at this point
  • Hopefully integrate fixes for Invalid frame header Plutonomicon/cardano-transaction-lib#803 because we're also seeing the issue described there
    • The issue isn't breaking the app though, so we could merge without it

@rynoV rynoV self-assigned this Jul 28, 2022
@rynoV rynoV linked an issue Jul 28, 2022 that may be closed by this pull request
@rynoV rynoV force-pushed the calum/ctl-breaking-changes branch from 2652218 to 62f5ec0 Compare July 28, 2022 22:42
rynoV added 4 commits July 29, 2022 09:15
CTL updates caused it to throw a bigger error than it was
before. Unsure how it wasn't throwing a bigger error before, the way
we were providing the function didn't make any sense
@rynoV rynoV marked this pull request as ready for review July 29, 2022 20:34
Copy link
Contributor

@samuelWilliams99 samuelWilliams99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume merging this will mean we'll need to update the ctl ref in the monorepo as well?

, image: params.imageUri
, mediaType: Nothing
, description: [ params.name, params.description ]
, description: Just params.description
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like an API change? will the frontend handle this correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, forgot about this, I'll make the update in this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description and name aren't actually parsed from the metadata, they're retrieved from the database. However the image field is parsed from the metadata, and that format changed from an array to a string, so I updated the parsing for it. According to the cip25 standard the image field can be a string or an array, so I made the parsing handle both possibilities, and this also means the nfts minted before this update are still useable

@rynoV
Copy link
Contributor Author

rynoV commented Aug 1, 2022

I assume merging this will mean we'll need to update the ctl ref in the monorepo as well?

It would probably be for the best. Maybe we should merge into some kind of staging branch instead?

@samuelWilliams99
Copy link
Contributor

Perhaps, but it's a relatively uncommon change, I think it's fine to just pr both of them
the subrepo should be commit locked anyway

@rynoV rynoV linked an issue Aug 1, 2022 that may be closed by this pull request
@rynoV rynoV requested a review from samuelWilliams99 August 1, 2022 23:23
{ caseArray =
( lmap (const "Image array does not contain a string") <<<
decodeAeson
) <=< note "Image field contains empty array" <<< head
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe in the CIP standard, long strings are split into arrays, is this how the image field works? or is each element intended to be a separate image?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For metadata created by us, we shouldn't be getting any images which are long strings split up, unless we change the length of our image value.

We could use CTL's cip25 modules to parse this and that should handle long strings being split into arrays, but then the NFTs minted previously would no longer be valid. I guess it's okay though, the old nfts are just for testing anyways

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need to support arbitrary cNFTs that adhere to the standard, as we won't always be the ones minting them. In future, artists will want a way to take their existing nft and import it to the protocol.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#42

@samuelWilliams99 samuelWilliams99 merged commit a6f2447 into master Aug 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update CTL and fix for breaking changes Use CTL's CIP25 metadata fixes Use CTL currentSlot

3 participants