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

Add transaction-payment runtime API V3 #5430

Merged
merged 6 commits into from
Jan 27, 2023

Conversation

notlesh
Copy link
Contributor

@notlesh notlesh commented Jan 19, 2023

This PR is a companion for paritytech/substrate#13110. It adds the same API definitions to pallet-transaction-payment introduced there. The API version is being rev'ed from V2 -> V3.

I have not been able to get this to work and could use a pointer.

When run my local changes here against a node/runtime supporting the new V3, the entire api.call.transactionPaymentApi is missing (undefined), likely the same symptom described in the PR above. It seems to work fine for V2.

I think I need to run yarn polkadot-types-from-chain to generate some code and put it in the right place, but I'm not sure exactly how to invoke this. I could be barking up the wrong tree, of course.

@jacogr
Copy link
Member

jacogr commented Jan 20, 2023

Weird, at quick glance it does add the right things. (The typegen is only needed for TS developers, so it doesn't need to be run)

How are you testing this against a local node? (Which steps?)

Copy link
Member

@jacogr jacogr left a comment

Choose a reason for hiding this comment

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

Some small linting fixes on quotes, aka ' vs "

packages/types/src/interfaces/payment/runtime.ts Outdated Show resolved Hide resolved
packages/types/src/interfaces/payment/runtime.ts Outdated Show resolved Hide resolved
Copy link
Member

@jacogr jacogr left a comment

Choose a reason for hiding this comment

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

Just spread into empty object (otherwise it would actually clobber the existing)

packages/types/src/interfaces/payment/runtime.ts Outdated Show resolved Hide resolved
Copy link
Member

@jacogr jacogr left a comment

Choose a reason for hiding this comment

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

Apply same "to-empty-spread" functionality for TransactionPaymentCallApi

packages/types/src/interfaces/payment/runtime.ts Outdated Show resolved Hide resolved
packages/types/src/interfaces/payment/runtime.ts Outdated Show resolved Hide resolved
@jacogr
Copy link
Member

jacogr commented Jan 20, 2023

After applying the above locally and running against your node -

image

image

So seems fine.

@notlesh
Copy link
Contributor Author

notlesh commented Jan 23, 2023

Thanks for the help, Jaco! I believe addressed the query_call_info issue, PTAL.

However, I haven't been able to set up an override for @polkadot.js/types to test this in any way. I've tried a few things but am really just shooting in the dark, is there some documentation on doing this? (I'm a typescript noob, btw).

@jacogr
Copy link
Member

jacogr commented Jan 24, 2023

Depends on how you want to test. So for my tests above, I have this structure - <ROOT>/api and <ROOT>/apps (i.e. both repos in the same top-level)

  1. Apply the changes in api
  2. inside api I do a yarn polkadot-dev-copy-to apps (this compiles all and replaces the node_modules inside apps)
  3. Inside apps, I do a yarn start
  4. Play with apps UI on localhost:3000

So basically the polkadot-dev-copy-to command is useful to get stuff into other projects. (The first .. is assumed, but any depth can be achieved, for sanity just made it simple to assume the same top-level structure in those scripts)

@notlesh
Copy link
Contributor Author

notlesh commented Jan 24, 2023

Thanks! I was able to test the changes, LGTM now!

Copy link
Member

@jacogr jacogr left a comment

Choose a reason for hiding this comment

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

Looks all-ok. Will wait for the Substrate PR to be merged before merging this one (just in-case there are some changes required there)

@ggwpez
Copy link

ggwpez commented Jan 26, 2023

Substrate just merged paritytech/substrate#13110

Copy link
Member

@jacogr jacogr left a comment

Choose a reason for hiding this comment

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

Thank you.

@jacogr jacogr merged commit cc4e4b2 into polkadot-js:master Jan 27, 2023
@polkadot-js-bot
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@polkadot-js polkadot-js locked as resolved and limited conversation to collaborators Jan 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants