-
Notifications
You must be signed in to change notification settings - Fork 151
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: query info feature detection #1305
Conversation
if ( | ||
!api.rpc.payment?.queryInfo && | ||
!api.call.transactionPaymentApi?.queryInfo | ||
) { | ||
extrinsics[idx].info = { | ||
error: 'Rpc method payment::queryInfo is not available', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR @xlc ! It looks good to me! I will just add here some of my thoughts :
- With this change, we do a complete check if
queryInfo
in general is available (either from runtime call or rpc) not just checking ifrpc
is available. - If we want this check, then we should change the error to something more generic like "The fee information is not available" (src)
- Also, if none of these methods is available then we should move this part of the code to an
else
statement after this check. Since it makes sense to call thefetchQueryInfo
function only if one of the two calls are available. - Additional secondary note : maybe we could also add the question mark wherever we call
apiAt.call.transactionPaymentApi.queryInfo
, e.g. this line or this line, etc. ? So it would beapiAt.call.transactionPaymentApi?.queryInfo
) as shown here in your PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional secondary note : maybe we could also add the question mark wherever we call apiAt.call.transactionPaymentApi.queryInfo, e.g. this line or this line, etc. ? So it would be apiAt.call.transactionPaymentApi?.queryInfo) as shown here in your PR.
This wont be necessary (in the context of this PR and not a refactor) since the check is already done before any of the calls are made.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thiink 1, 3, 4 are a bit unnecessary for this PR as its more of a fix per se and not a rewrite. But I do agree with point 2 where the error message could be a bit better, and not just specific for the rpc method.
I see what you are saying, and I agree it has merit. Happy to put it on the backlog, and hopefully we can get to it as well when things start to lighten up. |
The whole thing needs a refactoring so that the usage of API (i.e. fetchQueryInfo) is not detached to the API detection code but I don't all the time to do it so will just do the quick fix for now.