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

Make prepare_simulation_batch() more usable #32304

Merged
merged 3 commits into from
Jun 29, 2023

Conversation

ryoqun
Copy link
Member

@ryoqun ryoqun commented Jun 28, 2023

Problem

Summary of Changes

Fixes #

@codecov
Copy link

codecov bot commented Jun 28, 2023

Codecov Report

Merging #32304 (3287105) into master (4cfdb37) will increase coverage by 0.0%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##           master   #32304   +/-   ##
=======================================
  Coverage    82.0%    82.0%           
=======================================
  Files         773      773           
  Lines      209616   209619    +3     
=======================================
+ Hits       172038   172041    +3     
  Misses      37578    37578           

Copy link
Contributor

@apfitzge apfitzge left a comment

Choose a reason for hiding this comment

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

Change itself looks good. Slight hesitation on the naming, as there's no indication (just from the name) that this is for a single transaction "batch".

From the other fns in this area I'd assume I pass it a transactions: &'b [SanitizedTransaction],

wdyt?

@ryoqun
Copy link
Member Author

ryoqun commented Jun 29, 2023

Slight hesitation on the naming, as there's no indication (just from the name) that this is for a single transaction "batch".

From the other fns in this area I'd assume I pass it a transactions: &'b [SanitizedTransaction],

nice suggestion: fc8edca

@ryoqun ryoqun requested a review from apfitzge June 29, 2023 00:24
apfitzge
apfitzge previously approved these changes Jun 29, 2023
Copy link
Contributor

@apfitzge apfitzge left a comment

Choose a reason for hiding this comment

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

lgtm - though you might want to wait for @ilya-bobyr's review; I saw you explicitly requested his review and I just happen to notice the PR.

ilya-bobyr
ilya-bobyr previously approved these changes Jun 29, 2023
Copy link
Contributor

@ilya-bobyr ilya-bobyr left a comment

Choose a reason for hiding this comment

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

Somehow, this PR was not in my inbox %)
I must have opened it and then closed the tab by accident.
Looks good to me.

let mut batch = TransactionBatch::new(
vec![lock_result],
self,
Cow::Borrowed(std::slice::from_ref(transaction)),
Copy link
Contributor

Choose a reason for hiding this comment

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

minor

I think in most cases, elements are imported, up to a certain meaningful parent, rather than referenced with the full path. Even for std:

use std::slice;

And then

Suggested change
Cow::Borrowed(std::slice::from_ref(transaction)),
Cow::Borrowed(slice::from_ref(transaction)),

Copy link
Member Author

Choose a reason for hiding this comment

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

nice! done 3287105

@ryoqun ryoqun dismissed stale reviews from ilya-bobyr and apfitzge via 3287105 June 29, 2023 01:23
@ryoqun ryoqun requested a review from ilya-bobyr June 29, 2023 01:24
@ryoqun ryoqun merged commit 5b6c3ba into solana-labs:master Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants