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

Adds transaction payment rpc in minimal template #4516

Closed
wants to merge 1 commit into from

Conversation

gupnik
Copy link
Contributor

@gupnik gupnik commented May 18, 2024

No description provided.

@gupnik gupnik added the R0-silent Changes should not be mentioned in any release notes label May 18, 2024
@xlc
Copy link
Contributor

xlc commented May 18, 2024

why? we really should depreciate it instead. everyone should be using state_call

@gupnik
Copy link
Contributor Author

gupnik commented May 20, 2024

why? we really should depreciate it instead. everyone should be using state_call

So, the accounts tab in PJS apps doesn't work with the minimal template. I compared it with the solochain one and found this to be missing. Same is present in parachain template as well.

Are there nodes that use the state_call but work with PJS apps? If so, could you share an example please? If not, does it mean that we should change the implementation there instead?

@xlc
Copy link
Contributor

xlc commented May 20, 2024

it is pjs apps bug, lets fix it there

Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Please make an issue in PJS apps repo about this, but in the meantime do merge this PR please so the templates are use-able.

The broad context is to always call into runtime APIs via state_call and not RPC. This means both for tx payment and system nonce, or anything else. This only applies to RPC that only forward the call to the runtime API and do nothing more.

@kianenigma
Copy link
Contributor

Ideally the api.rpc.foo calls will be auto marked as deprecated with a proper message on what to do instead. This can be done as a consequence of us marking the rust APIs as deprecated.

Then, after a few months, we can actually start removing the RPCs from the node sides.

@xlc
Copy link
Contributor

xlc commented May 20, 2024

adding this to template will only make the deprecation process more painful as new projects using the template will inherit it and then they also need to remove those in future.
on the other hand, it shouldn’t take more than a day of effort to fix pjs apps. it was an easy task for me to fix it for sidecar paritytech/substrate-api-sidecar#1169

@kianenigma
Copy link
Contributor

Okay, I was under the wrong impression that we need this template next week for PBA, but actually we have a private fork there and can "temporarily" fix things in there, so I would have less of a strong opinion here. I would still not object to merging this though, if it gains enough support.

@gupnik please proceed as discussed with a more ground up fix and see where this goes. cc @TarikGul

@gupnik
Copy link
Contributor Author

gupnik commented May 21, 2024

On setting up a local instance of the PJS apps, I was able to drill this further and it turned out that the issue is actually fixed by polkadot-js/apps#10603. Not sure why it occurred only for omni node based on the minimal fork though.

I do see that PJS apps is already using api.call.transactionPaymentApi. So, it seems that this change should not be needed. Hence, closing this PR. Thanks @xlc @kianenigma.

@gupnik gupnik closed this May 21, 2024
@bkchr bkchr deleted the gupnik/minimal-template-fix branch May 21, 2024 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants