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: add query param metadata #746

Merged
merged 14 commits into from
Nov 9, 2021
Merged

feat: add query param metadata #746

merged 14 commits into from
Nov 9, 2021

Conversation

TarikGul
Copy link
Member

@TarikGul TarikGul commented Nov 1, 2021

closes: #476

Pertains to /transaction/material

This adds the query param metadata that is acting as a replacement for the noMeta query param.

metadata will take in a format for what type of data you want back. It accepts 'json' or 'scale'.
json -> decoded metadata as json
scale -> SCALE encoded hex value of the metadata

noMeta will still be available for 2 more weekly releases then its subject to be removed. The currrent default behavior is that when there is no query param scale encoded metadata is returned. This will change when this is deprecated, and the default behavior will be no metadata unless the metadata field is specified.

docs/src/openapi-v1.yaml Outdated Show resolved Hide resolved
docs/src/openapi-v1.yaml Outdated Show resolved Hide resolved
Comment on lines 80 to 94
const metadataArg =
!metadata || metadata === 'json' || metadata === 'scale';

if (!metadataArg)
throw new Error('Invalid inputted value for the `metadata` query param.');
if (metadata === 'json') return 'json';
if (!noMeta || noMeta === 'false' || metadata === 'scale') return 'scale';
if (noMeta === 'true') {
Log.logger.warn(
'`noMeta` query param will be deprecated in sidecar v13, and replaced with `metadata` please migrate'
);
return false;
}

return 'scale';
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was pondering whether this could be simplified. The best I came up with was:

if (!noMeta && metadata === 'json')
    return 'json';
if (!noMeta && metadata === 'scale')
    return 'scale';
if (!noMeta && metadata)
    throw new Error('Invalid inputted value for the `metadata` query param.');

if (noMeta) {
    Log.logger.warn(
        '`noMeta` query param will be deprecated in sidecar v13, and replaced with `metadata` please migrate'
    );
}
return false;

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea I agree, when I wrote this I knew it was really late, and super messy looking and was in need of a re-write 😅 . Thank you for working through that, I had a lot of trouble grouping the arguments together. I agree the above is significantly better and ill update it!

Copy link
Member Author

@TarikGul TarikGul Nov 4, 2021

Choose a reason for hiding this comment

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

So the only issue with the logic above is that we need to account for when no query params are inputted the default behavior is to have scale metadata returned.

/**
* Checks to see if the `metadata` query param is inputted, if it isnt,
* it will default to the old behavior. This is to be removed once after 
* the `noMeta` query param is fully deprecated.
*/
if (metadata) {
    switch (metadata) {
        case('json'):
            return 'json';
        case('scale'):
            return 'scale';
        default:
            throw new Error('Invalid inputted value for the `metadata` query param.');
    }
}

if (noMeta) {
    Log.logger.warn(
        '`noMeta` query param will be deprecated in sidecar v13, and replaced with `metadata` please migrate'
    );
    switch (noMeta) {
        case('true'):
            return false;
        case('false'):
            return 'scale';
    }
}

// default behavior until `noMeta` is deprecated, then false will be default
return 'scale';

Copy link
Member Author

Choose a reason for hiding this comment

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

So i updated the logic a little, and kept the same format :). Thanks for making this easy for me to understand!

Copy link
Member Author

Choose a reason for hiding this comment

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

Also the idea here being that we want the metadata query param to override noMeta if its used

Copy link
Collaborator

Choose a reason for hiding this comment

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

Aaah overriding noMeta, got it! your new version looks clean :)

@TarikGul TarikGul changed the title feat: add query param to decode metadata feat: add query param metadata Nov 9, 2021
@TarikGul TarikGul merged commit 273cac2 into master Nov 9, 2021
@TarikGul TarikGul deleted the tarik-tx-mat branch November 9, 2021 14:47
This pull request was closed.
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.

/transaction/material enhancement
3 participants