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.

Sign up for GitHub

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

Jump to bottom

fix: query info feature detection #1305

Merged
merged 1 commit into from
Aug 2, 2023
+4 −1
Merged

fix: query info feature detection #1305

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/services/blocks/BlocksService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,10 @@ export class BlocksService extends AbstractService {
continue;
}

if (!api.rpc.payment || !api.rpc.payment.queryInfo) {
if (
!api.rpc.payment?.queryInfo &&
!api.call.transactionPaymentApi?.queryInfo
) {
extrinsics[idx].info = {
error: 'Rpc method payment::queryInfo is not available',
Copy link
Contributor

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 :

  1. With this change, we do a complete check if queryInfo in general is available (either from runtime call or rpc) not just checking if rpc is available.
  2. If we want this check, then we should change the error to something more generic like "The fee information is not available" (src)
  3. 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 the fetchQueryInfo function only if one of the two calls are available.
  4. 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.

Copy link
Member

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.

Copy link
Member

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.

};
Expand Down
Loading