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 getTransactions web3.js method #26099

Merged

Conversation

kunaljaydesai
Copy link
Contributor

Problem

getTransactions parses the transaction response differently from getTransaction, but they return the same type. Specifically, transaction.message is not of the type Message.

Summary of Changes

Pulls out the correct message type for each transaction in getTransactions call

cc @joncinque would you be able to review this for me? thanks!

@kunaljaydesai kunaljaydesai changed the title Kunal/fix batch transactions Fix getTransactions web3.js method Jun 21, 2022
@mergify mergify bot added the community Community contribution label Jun 21, 2022
@mergify mergify bot requested a review from a team June 21, 2022 15:58
steveluscher
steveluscher previously approved these changes Jun 21, 2022
Copy link
Contributor

@steveluscher steveluscher left a comment

Choose a reason for hiding this comment

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

We are so early, still.

Thank you for this fix!

@@ -3566,7 +3566,16 @@ export class Connection {
if ('error' in res) {
throw new Error('failed to get transactions: ' + res.error.message);
}
return res.result;
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow. Somehow TypeScript, despite the fact that it knows the type of res.result, typed the outer resasany`.

image

I wonder if this is because they have the same name (the variable name is shadowed).

Copy link
Contributor

Choose a reason for hiding this comment

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

Found out the root cause of this. #26102

Comment on lines +3572 to +3578
return {
...result,
transaction: {
...result.transaction,
message: new Message(result.transaction.message),
},
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Slight preference for pulling this out into a method (getTransactionResponseFromGetTransactionsResult()so thatgetTransactionandgetTransactions` can share it, but no big deal – what you've done here is more readable.

@@ -2197,6 +2197,138 @@ describe('Connection', function () {
expect(nullResponse).to.be.null;
});

it('get transactions', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

<3

@steveluscher steveluscher added the javascript Pull requests that update Javascript code label Jun 21, 2022
@steveluscher steveluscher force-pushed the kunal/fix-batch-transactions branch from 64d5ae5 to 5848e7b Compare June 21, 2022 17:15
@mergify mergify bot dismissed steveluscher’s stale review June 21, 2022 17:15

Pull request has been modified.

@codecov
Copy link

codecov bot commented Jun 21, 2022

Codecov Report

Merging #26099 (daef70e) into master (8caced6) will decrease coverage by 6.3%.
The diff coverage is n/a.

❗ Current head daef70e differs from pull request most recent head 5848e7b. Consider uploading reports for the commit 5848e7b to get more accurate results

@@             Coverage Diff             @@
##           master   #26099       +/-   ##
===========================================
- Coverage    82.1%    75.7%     -6.4%     
===========================================
  Files         628       40      -588     
  Lines      171471     2348   -169123     
  Branches        0      339      +339     
===========================================
- Hits       140878     1779   -139099     
+ Misses      30593      450    -30143     
- Partials        0      119      +119     

@steveluscher steveluscher merged commit aea84e6 into solana-labs:master Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Community contribution javascript Pull requests that update Javascript code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants