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

account_saver moved to runtime #2773

Merged
merged 1 commit into from
Aug 30, 2024

Conversation

apfitzge
Copy link

Problem

  • account_saver is only used in runtime, not in svm.

Summary of Changes

  • move account_saver.rs to runtime from svm

Fixes #

@apfitzge apfitzge marked this pull request as ready for review August 29, 2024 12:58
@jstarry
Copy link

jstarry commented Aug 29, 2024

I don't feel strongly either way, I'll leave it to @buffalojoec for approval.

buffalojoec
buffalojoec previously approved these changes Aug 30, 2024
Copy link

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

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

Thank you!! In my opinion, this makes sense to move into runtime. It's a post-processing step that should not be imposed on off-chain consumers of the SVM API.

@apfitzge
Copy link
Author

rebased due to conflict on svm lib modules

@apfitzge apfitzge requested a review from buffalojoec August 30, 2024 13:03
@apfitzge apfitzge added automerge automerge Merge this Pull Request automatically once CI passes and removed automerge automerge Merge this Pull Request automatically once CI passes labels Aug 30, 2024
@apfitzge apfitzge merged commit b345960 into anza-xyz:master Aug 30, 2024
42 checks passed
@apfitzge apfitzge deleted the move_accounts_saver branch August 30, 2024 15:06
@2501babe
Copy link
Member

2501babe commented Sep 7, 2024

would this be a bad time to note that i was working on #2741 to make account_saver::collect_accounts_to_store viable to use in svm 😅

@apfitzge
Copy link
Author

apfitzge commented Sep 9, 2024

would this be a bad time to note that i was working on #2741 to make account_saver::collect_accounts_to_store viable to use in svm 😅

Sorry, I'm missing where its' used inside svm in that PR, could you provide a direct link?

@2501babe
Copy link
Member

sorry, i should have been clearer, #2741 doesnt use account saver in svm, it is preparation for doing so in #2702. will follow up with more detail so im not spamming a closed pr, just wanted to mention it here in case anyone might object to me reverting this in the near future

@apfitzge
Copy link
Author

will follow up with more detail so im not spamming a closed pr, just wanted to mention it here in case anyone might object to me reverting this in the near future

I'm inclined to oppose reverting this as is; I think I'd rather see this reworked for what we want than move it back as-is to svm.

Right now it does two distinct things, which are the return values: ( Vec<(&'a Pubkey, &'a AccountSharedData)>, Option<Vec<&'a SanitizedTransaction>>, ).

  1. It collects a list of Pubkey and the associated AccountSharedData for that Pubkey.
  2. It (optionally) collects transaction references to eventually pass to the geyser interface
    • This definitely has no place in SVM imo

They seem tied together right now because the filtering is identical. But I think the filtering is quite cheap (&&-ing a few bools?) - we could just redo it if needed for geyser when constructing interfaces to pass there?

Trying to think of a good way to make these separate but without duplicating the logic...

ray-kast pushed a commit to abklabs/agave that referenced this pull request Nov 27, 2024
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.

4 participants