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!: removes controller arg from bond and setController methods #309

Merged
merged 2 commits into from
Jul 3, 2023

Conversation

Imod7
Copy link
Collaborator

@Imod7 Imod7 commented Jul 1, 2023

Description

Following the controller changes in the substrate PR (related forum post), we also update the same methods in txwrapper-core :

  • bond and
  • setController

by removing the controller argument.

Additional Metadata

Westend Metadata v9430 were added so we can test the changes in the updated methods (bond and setController).

Tests

The expected results/hex calls in bond.spec.ts and setController.spec.ts were updated accordingly.
The new hex calls

  • 0x0600910100 for the bond test
  • 0x0608 for the setController test

can be double checked in pjs-apps > Decode section (connected to Westend) where you can see that the controller argument is no longer present.

@Imod7 Imod7 requested a review from a team as a code owner July 1, 2023 17:06
Copy link
Member

@TarikGul TarikGul left a comment

Choose a reason for hiding this comment

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

lgtm Great Job

@Imod7 Imod7 merged commit f951f56 into main Jul 3, 2023
@Imod7 Imod7 deleted the domi-remove-controller-arg branch July 3, 2023 14:57
Copy link
Contributor

@EmilIvanichkovv EmilIvanichkovv left a comment

Choose a reason for hiding this comment

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

Today, as I was in the process of preparing a Pull Request to add the Westend metadata and registry (needed it for offline decoding of unsigned transactions in Westend), I noticed that you have already merged and released the changes. It's wonderful to see that the feature I needed was implemented on the very same day!

However, I do have one question or concern. Shouldn't the tokenDecimals value for Westend be 12 instead of 10? I have provided a review on the Pull Request and have also created a separate Pull Request to address this issue. I based my understanding on the information outlined in the following resource: https://wiki.polkadot.network/docs/learn-DOT#getting-tokens-on-the-westend-testnet.

Thank you for your attention and great work!

return mockGetRegistryBase({
chainProperties: {
ss58Format: 0,
tokenDecimals: 10,
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't the value of tokenDecimals need to be 12 instead of 10?

Copy link
Member

Choose a reason for hiding this comment

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

Totally correct, I also added a comment in the PR you added. I really appreciate the thorough check, and thank you so much!

Copy link
Member

Choose a reason for hiding this comment

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

Great catch :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

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.

4 participants