Skip to content
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: project missing handle props #905

Merged

Conversation

mkazlauskas
Copy link
Member

@mkazlauskas mkazlauskas commented Sep 6, 2023

Context

TypeormHandleProvider can't resolve some required handle properties:

  • default handle for stake and payment credentials
  • images: image/pfp/bg

LW-6928

Proposed Solution

See commit log

Important Changes Introduced

that can be used to look up the label and decoded value by asset id
to not clash with Handle type from core package
fix a bug where it would incorrectly map cip68 handle names
by including label as part of the Handle

also, add optional 'logger' parameter to 'withHandles'
operator (to be converted to required)

BREAKING CHANGE: withHandles now requires WithCIP67 props in
also clean up the test by removing duplicate data
@mkazlauskas mkazlauskas force-pushed the feat/project-missing-handle-props branch 2 times, most recently from 6ffde67 to d1cf475 Compare September 6, 2023 08:00
there can be multiple 'copies' of nft metadata for an asset in a block

it was keeping the 1st one instead
hoist from NftMetadata.fromPlutusData and add tests
@mkazlauskas mkazlauskas force-pushed the feat/project-missing-handle-props branch 2 times, most recently from b8c3f02 to f097bb4 Compare September 7, 2023 06:05
Copy link
Member

@rhyslbw rhyslbw left a comment

Choose a reason for hiding this comment

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

Great work here @mkazlauskas 🧙‍♂️

It's possible this will address the discrepancy @VanessaPC found when comparing results of our projection vs the Kora Labs API right?

@mkazlauskas mkazlauskas force-pushed the feat/project-missing-handle-props branch from 446ce7b to 37e7fec Compare September 7, 2023 12:03
Copy link
Contributor

@iccicci iccicci left a comment

Choose a reason for hiding this comment

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

LGTM!

Just some ideas about readability...

also add missing entity inter-dependency output->tokens
it would omit the prop if no stake pools of some status are in the database
in reality those shouldn't be in user's wallet, but technically they can be
to be used when generating cardano-services db snapshot
add a cip68 handle to handle test db

disable some flaky tests: there are no 'retiring' pools in db

fix TypeormStakePoolProvider test setup to match partial strings
…prop

it does not add any new columns, but simplifies queries
@mkazlauskas mkazlauskas force-pushed the feat/project-missing-handle-props branch from a1061ae to 559f57d Compare September 11, 2023 21:08
@mkazlauskas mkazlauskas merged commit 1c929dc into feat/plutus-data-serialization Sep 12, 2023
3 checks passed
@mkazlauskas mkazlauskas deleted the feat/project-missing-handle-props branch September 12, 2023 07:21
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.

3 participants