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

Create runtime-transaction crate to build transaction types with state for runtime use #33471

Conversation

tao-stones
Copy link
Contributor

@tao-stones tao-stones commented Sep 29, 2023

Problem

Add a transaction type for runtime that contains encapsulated internal metadata, which can be accessed/updated as sanitized transaction goes through execution pipeline.

It is an Alternative to #33418

Summary of Changes

  • create a crate for runtime-transaction type to reduce dependencies (registered it at crates.io)
  • add RuntimeTransaction structs that representing 2 (currently defined) states
  • add TransactionMeta currently to match sdk::SanitizedTransaction. Will be expanded later
  • add TransactionMeta Traits, each for one transaction state

Fixes #

@tao-stones tao-stones force-pushed the runtime-transaction-state-machine-via-trait branch 2 times, most recently from 0ca1826 to 09682ad Compare October 2, 2023 16:52
@codecov
Copy link

codecov bot commented Oct 2, 2023

Codecov Report

Merging #33471 (5b0c1d6) into master (061883e) will increase coverage by 0.0%.
Report is 2 commits behind head on master.
The diff coverage is 28.7%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #33471   +/-   ##
=======================================
  Coverage    81.9%    81.9%           
=======================================
  Files         812      816    +4     
  Lines      219645   219688   +43     
=======================================
+ Hits       179922   180003   +81     
+ Misses      39723    39685   -38     

@tao-stones tao-stones force-pushed the runtime-transaction-state-machine-via-trait branch 2 times, most recently from c9d6796 to 9f6da2c Compare October 4, 2023 16:29
@tao-stones tao-stones changed the title Runtime transaction state machine via trait Create runtime-transaction crate to build transaction types with state for runtime use Oct 4, 2023
@tao-stones tao-stones marked this pull request as ready for review October 4, 2023 16:51
@tao-stones tao-stones force-pushed the runtime-transaction-state-machine-via-trait branch 2 times, most recently from 3da7d1f to 294382c Compare October 5, 2023 15:27
@tao-stones tao-stones force-pushed the runtime-transaction-state-machine-via-trait branch from 294382c to edac647 Compare October 19, 2023 22:35
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.

Liking the overall approach here with wire -> sanitized -> tx w/ static -> tx w/ dynamic

runtime-transaction/build.rs Outdated Show resolved Hide resolved
runtime-transaction/src/simple_vote_transaction_checker.rs Outdated Show resolved Hide resolved
runtime-transaction/src/simple_vote_transaction_checker.rs Outdated Show resolved Hide resolved
runtime-transaction/src/transaction_meta.rs Show resolved Hide resolved
runtime-transaction/src/transaction_meta.rs Show resolved Hide resolved
@tao-stones tao-stones force-pushed the runtime-transaction-state-machine-via-trait branch from f48d15d to 90dc6ef Compare November 6, 2023 18:08
@tao-stones tao-stones force-pushed the runtime-transaction-state-machine-via-trait branch from 90dc6ef to 43eda39 Compare November 15, 2023 23:33
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 - couple of minor things I will follow-up on myself so we can begin using this earlier!

@tao-stones tao-stones merged commit e790f09 into solana-labs:master Nov 16, 2023
44 checks passed
@@ -32,6 +32,11 @@ impl SanitizedVersionedTransaction {
pub fn get_message(&self) -> &SanitizedVersionedMessage {
&self.message
}

/// Consumes the SanitizedVersionedTransaction, returning the fields individually.
pub fn destruct(self) -> (Vec<Signature>, SanitizedVersionedMessage) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this could be an impl From<SanitizedVersionedTransaction> for (Vec<Signature>, SanitizedVersionedMessage) if not make the fields pub instead of pub (crate) to allow in place destructuring

Copy link
Contributor

Choose a reason for hiding this comment

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

Fields on SanitizedVersionedMessage should not be pub. Them being private is intentional, see the previous discussion on it: #33471 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

From impl, then. i would assume a method called destruct() to do something... destructive.

runtime-transaction/src/lib.rs Show resolved Hide resolved
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