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.

Sign up for GitHub

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

Jump to bottom

Temporarily use metadata revision branch of cardano-api #2131

Merged
merged 9 commits into from
Sep 11, 2020
Merged

Temporarily use metadata revision branch of cardano-api #2131

merged 9 commits into from
Sep 11, 2020

Conversation

KtorZ
Copy link
Member

@KtorZ KtorZ commented Sep 10, 2020

Issue Number

Overview

  • 3698a42
    📍 use temporary patched cardnao-api-1.19.1 with metadata support

  • 29050f7
    📍 use released version (2.0.0) for cardano-addresses

  • 5dcbee6
    📍 revise metadata generator to be generated from valid JSON values

Comments

We agreed with @AlexIOHK to move on with that version (so called "B") until the final version (describing metadata as JSON, so called "C") is finalized.

@KtorZ KtorZ requested review from rvl and AlexIOHK September 10, 2020 17:05
@KtorZ KtorZ self-assigned this Sep 10, 2020
AlexIOHK
AlexIOHK approved these changes Sep 10, 2020
View reviewed changes
Copy link

@AlexIOHK AlexIOHK left a comment

Choose a reason for hiding this comment

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

Agreed with using option "B" until further improvements are made.

@rvl rvl added the IMPROVEMENT Mark a PR as an improvement, for auto-generated CHANGELOG label Sep 11, 2020
@rvl rvl added this to the (ADP-307) Transaction metadata milestone Sep 11, 2020
rvl
rvl approved these changes Sep 11, 2020
View reviewed changes
Copy link
Contributor

@rvl rvl left a comment

Choose a reason for hiding this comment

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

But this scheme makes testing difficult for us. For example, if a metadata value goes through the database, that will be a json roundtrip, and the property tests will find the counterexamples which don't roundtrip properly.

This was referenced Sep 11, 2020
Closed
Closed
@KtorZ KtorZ force-pushed the KtorZ/metadata-temporary-update-for-release branch from 9721d5e to b89ec3a Compare September 11, 2020 08:59
@KtorZ
Copy link
Member Author

KtorZ commented Sep 11, 2020

bors r+

@KtorZ KtorZ mentioned this pull request Sep 11, 2020
Merged
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Sep 11, 2020

Build succeeded:

@iohk-bors iohk-bors bot merged commit ea7b585 into master Sep 11, 2020
@iohk-bors iohk-bors bot deleted the KtorZ/metadata-temporary-update-for-release branch September 11, 2020 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlexIOHK AlexIOHK AlexIOHK approved these changes

@rvl rvl rvl approved these changes

Assignees

@KtorZ KtorZ

Labels
IMPROVEMENT Mark a PR as an improvement, for auto-generated CHANGELOG
Projects
None yet
Milestone
(ADP-307) Transaction metadata
Development

Successfully merging this pull request may close these issues.
3 participants
@KtorZ @rvl @AlexIOHK