Skip to content

fix(metadata): PSGS-934 fixed broken TxMetadataOut json decoder#59

Open
mpskowron wants to merge 1 commit into
masterfrom
fix/PSGS-934_metadata_decoder
Open

fix(metadata): PSGS-934 fixed broken TxMetadataOut json decoder#59
mpskowron wants to merge 1 commit into
masterfrom
fix/PSGS-934_metadata_decoder

Conversation

@mpskowron

Copy link
Copy Markdown
Contributor

No description provided.

@mpskowron mpskowron requested review from adamsmo and chuchulo January 27, 2023 10:58

@adamsmo adamsmo left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This will create corner cases and create dependency in our code on the exact implementation that is provided by wallet
We discussed it yesterday, one option was to return metadata provided as input, another one was to just ask graphql for the metadata of mined transaction, in both cases there is no changes done in psg in the JSON

@adamsmo

adamsmo commented Jan 27, 2023

Copy link
Copy Markdown

what prevents you from just calling getMetadata() ?

@adamsmo

adamsmo commented Jan 27, 2023

Copy link
Copy Markdown

Looks like tests are failing because of not enough funds on the wallet

@mpskowron

mpskowron commented Jan 27, 2023

Copy link
Copy Markdown
Contributor Author

what prevents you from just calling getMetadata() ?

Probably nothing, after that first solution didn't work, I've seen that I might have hard time fixing tests if I won't fix the decoder, thus thought that it will be done faster if I do it. I might have been wrong with that. I also thought about how unstable GraphQl, thus not using it might give user a better experience. I get your point about more corner cases. I'll just call that getMetadata if parsing json fails - this seems like the best solution. What do you think?

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.

2 participants