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

fix!: change the response for vesting-info #717

Merged
merged 8 commits into from
Oct 14, 2021
Merged

Conversation

TarikGul
Copy link
Member

@TarikGul TarikGul commented Oct 14, 2021

BREAKING CHANGE

With runtime v9110, the vesting-info response now returns an array for vesting, with the VestingInfo type now being Vec<PalletVestingVestingInfo>. Sidecar previously returned responses under the vesting key as objects, therefore when there was no information the response was an empty object.

This is now changed as the response will default to an empty array when there is no vesting-info on the account, and the past responses that were objects, will now be wrapped in an array.

Examples:

Before:

{
  "at": {
    "hash": "0x89f4608b568a05a89737bc826a32f8677f2646a5773312d7dee3a667458e5a98",
    "height": "6000000"
  },
  "vesting": {
      "locked": "1749990000000000",
      "perBlock": "166475460",
      "startingBlock": "4961000"
    }
}

After

{
  "at": {
    "hash": "0x89f4608b568a05a89737bc826a32f8677f2646a5773312d7dee3a667458e5a98",
    "height": "6000000"
  },
  "vesting": [
    {
      "locked": "1749990000000000",
      "perBlock": "166475460",
      "startingBlock": "4961000"
    }
  ]
}

@TarikGul
Copy link
Member Author

This PR will be followed up by #709

Copy link
Contributor

@maciejhirsz maciejhirsz left a comment

Choose a reason for hiding this comment

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

Tiny nit

Comment on lines 33 to 35
: Array.isArray(vesting.unwrap())
? vesting.unwrap()
: [vesting.unwrap()],
Copy link
Contributor

Choose a reason for hiding this comment

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

Would like to avoid double unwrap here, but not sure what a good way would be, TS would likely scream about something like:

Suggested change
: Array.isArray(vesting.unwrap())
? vesting.unwrap()
: [vesting.unwrap()],
: Array.isArray(vesting = vesting.unwrap())
? vesting
: [vesting],

Could use a different variable for the unwrapped value (declared before the return, assigned in place)? 🤔

Copy link
Member Author

@TarikGul TarikGul Oct 14, 2021

Choose a reason for hiding this comment

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

Ahh very good point. I should have noticed that earlier when I tried to abstract the vesting.unwrap() before calling vesting.isNone which caused me issues.

Good call. Going to declare 2 variables, one for the bool value, and the other to unwrap vesting

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 actually changed it quiet a bit to isolate each case more, it avoids unwrapping if isNone is true and also avoids unwrapping multiple times as well.

Comment on lines +29 to +40
if (vesting.isNone) {
return {
at,
vesting: [],
};
} else {
const unwrapVesting = vesting.unwrap();

return {
at,
vesting: Array.isArray(unwrapVesting) ? unwrapVesting : [unwrapVesting],
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Optimizing for clarity, I like it.

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.

None yet

4 participants