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

Add (preflight) simulation to BanksClient #22084

Merged
merged 7 commits into from
Dec 28, 2021
Merged

Add (preflight) simulation to BanksClient #22084

merged 7 commits into from
Dec 28, 2021

Conversation

CriesofCarrots
Copy link
Contributor

Problem

BanksServer operation is confusing or clunky for cases where a transaction does not land (successfully or with InstructionError). See #22029 for more details and examples.

Summary of Changes

Add Banks method with preflight checks, and expose via the BanksClient::process_transaction_with_preflight() method.
Consider adding stronger comments recommending this api.

Fixes #22029

cc @joncinque

@codecov
Copy link

codecov bot commented Dec 23, 2021 •
edited
Loading

Codecov Report

Merging #22084 (00d004f) into master (e61a736) will decrease coverage by 0.0%.
The diff coverage is 12.3%.

@@            Coverage Diff            @@
##           master   #22084     +/-   ##
=========================================
- Coverage    81.2%    81.1%   -0.1%     
=========================================
  Files         521      521             
  Lines      146060   146126     +66     
=========================================
+ Hits       118630   118632      +2     
- Misses      27430    27494     +64

joncinque
joncinque reviewed Dec 23, 2021
View reviewed changes
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.

Just a few thoughts, looking good so far!

banks-client/src/error.rs Show resolved Hide resolved
banks-client/src/lib.rs Outdated Show resolved Hide resolved
sdk/src/transaction/sanitized.rs Outdated Show resolved Hide resolved
banks-interface/src/lib.rs Outdated Show resolved Hide resolved
@joncinque joncinque mentioned this pull request Dec 24, 2021
Merged
@CriesofCarrots CriesofCarrots force-pushed the banks-simulate branch 2 times, most recently from 9e5cc71 to e83f548 Compare December 27, 2021 20:57
@CriesofCarrots CriesofCarrots marked this pull request as ready for review December 27, 2021 20:58
@CriesofCarrots
Copy link
Contributor Author

@joncinque , ready for proper review! I verified this against the case that came up in solana-program-library/token/rust/tests and this works well. Do you have any code for the cases you mentioned here #22029 (comment) that I could also verify this against?

joncinque
joncinque previously approved these changes Dec 28, 2021
View reviewed changes
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.

Looks great! Just one tiny nit, which you can absolutely take or leave

banks-interface/src/lib.rs Outdated Show resolved Hide resolved
@mergify mergify bot dismissed joncinque’s stale review December 28, 2021 17:29

Pull request has been modified.

@CriesofCarrots CriesofCarrots added the automerge Merge this Pull Request automatically once CI passes label Dec 28, 2021
@mergify mergify bot merged commit 422a095 into solana-labs:master Dec 28, 2021
@mergify mergify bot mentioned this pull request Dec 28, 2021
Merged
mergify bot pushed a commit that referenced this pull request Dec 28, 2021
@CriesofCarrots
Add (preflight) simulation to BanksClient (#22084)
4083dc4 
* Add more-legitimate conversion from legacy Transaction to SanitizedTransaction

* Add Banks method with preflight checks

* Expose BanksClient method with preflight checks

* Unwrap simulation err

* Add Bank simulation method that works on unfrozen Banks

* Add simpler api

* Better name: BanksTransactionResultWithSimulation

(cherry picked from commit 422a095)
mergify bot added a commit that referenced this pull request Dec 28, 2021
@mergify @CriesofCarrots
Add (preflight) simulation to BanksClient (#22084) (#22149)
adc584e 
* Add more-legitimate conversion from legacy Transaction to SanitizedTransaction

* Add Banks method with preflight checks

* Expose BanksClient method with preflight checks

* Unwrap simulation err

* Add Bank simulation method that works on unfrozen Banks

* Add simpler api

* Better name: BanksTransactionResultWithSimulation

(cherry picked from commit 422a095)

Co-authored-by: Tyera Eulberg <teulberg@gmail.com>
This was referenced Dec 29, 2021
Closed
Closed
@joncinque
Copy link
Contributor

Do you have any code for the cases you mentioned here #22029 (comment) that I could also verify this against?

Oops, I never responded to this part. I didn't have any specific test cases unfortunately

@CriesofCarrots
Copy link
Contributor Author

Oops, I never responded to this part. I didn't have any specific test cases unfortunately

No worries, wrote my own post-merge.

@brooksprumo brooksprumo mentioned this pull request Jan 5, 2022
Closed
@CriesofCarrots CriesofCarrots deleted the banks-simulate branch February 26, 2022 21:03
@github-actions
Copy link
Contributor

This PR has been automatically locked since there has not been any activity in past 14 days after it was merged.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@joncinque joncinque joncinque left review comments

Assignees
No one assigned
Labels
automerge Merge this Pull Request automatically once CI passes locked PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

BanksServer doesn't simulate transactions
2 participants
@CriesofCarrots @joncinque