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

Support for returnData in simulate transaction #26499

Merged
merged 1 commit into from
Jul 12, 2022

Conversation

atharmohammad
Copy link
Contributor

The PR adds --
1). support for returnData to SimulatedTransactionResponse with a unit test.
2). return test data in noop program to read the data back from the returnData field in the response.
3). live test with test-validator, simulating transaction on noop program to read the returnData.

cc: @joncinque

@mergify mergify bot added the community Community contribution label Jul 8, 2022
@mergify mergify bot requested a review from a team July 8, 2022 12:00
@atharmohammad atharmohammad marked this pull request as ready for review July 8, 2022 12:00
if (mockServer) {
it('returnData on simulateTransaction', async () => {
const tx = new Transaction();
tx.feePayer = Keypair.generate().publicKey;
Copy link
Contributor

Choose a reason for hiding this comment

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

@joncinque It's worth noting that this will only work with a funded feePayer, which makes simulation somewhat problematic for second-order return-data apis. That doesn't need to hold up anything here, but just something to think about for usage and documentation purposes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah true, I was thinking we could at least support it in its current state and improve on it later. Just to be totally clear, when you say "that doesn't need to hold up anything here", do you mean that you're ok with the approach?

Also, did we ever come up with a model for simulating transactions without a funded fee payer?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, I’m fine with this approach.
No, we haven’t designed implemented a way to bypass fee-payer check in simulation

Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Great job -- looks really close! Mainly some small things to clean up, then we can merge this

web3.js/src/connection.ts Outdated Show resolved Hide resolved
web3.js/src/connection.ts Outdated Show resolved Hide resolved
web3.js/test/bpf-loader.test.ts Outdated Show resolved Hide resolved
web3.js/test/bpf-loader.test.ts Outdated Show resolved Hide resolved
web3.js/test/bpf-loader.test.ts Show resolved Hide resolved
web3.js/test/connection.test.ts Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 11, 2022

Codecov Report

Merging #26499 (616ced2) into master (1165a7f) will decrease coverage by 4.4%.
The diff coverage is n/a.

❗ Current head 616ced2 differs from pull request most recent head 156affc. Consider uploading reports for the commit 156affc to get more accurate results

@@             Coverage Diff             @@
##           master   #26499       +/-   ##
===========================================
- Coverage    81.9%    77.4%     -4.5%     
===========================================
  Files         631       40      -591     
  Lines      174252     2370   -171882     
  Branches        0      343      +343     
===========================================
- Hits       142728     1836   -140892     
+ Misses      31524      406    -31118     
- Partials        0      128      +128     

nullable(
pick({
programId: string(),
data: tuple([string(), literal('base64')]),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

need to hard code base64 here since TransactionReturnDataEncoding is a type and was giving error, don't know if there is any way to avoid this.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can eventually use some other type, but no problem with doing that here

@joncinque
Copy link
Contributor

Looks great, thanks for the contribution!

@joncinque joncinque merged commit d0d3ad7 into solana-labs:master Jul 12, 2022
Lcchy pushed a commit to Bonfida/solana that referenced this pull request Jul 22, 2022
feat(web3.js): add Support for returnData in simulate transaction
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Community contribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants