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

feat: builder fns for PrivateTransactionRequest and inner props (#1954) #2023

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

nadtech-hub
Copy link
Contributor

@nadtech-hub nadtech-hub commented Feb 7, 2025

Motivation

Builder style functions for PrivateTransactionRequest

Solution

Added PrivateTransactionRequest's and inner properties implementation blocks containing builder fns

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

@nadtech-hub
Copy link
Contributor Author

afaik, you lot don't cover such functionality with tests, do you?

Signed-off-by: Aliaksei Misiukevich <[email protected]>
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

cool, smol nit

crates/rpc-types-mev/src/eth_calls.rs Outdated Show resolved Hide resolved
crates/rpc-types-mev/src/eth_calls.rs Outdated Show resolved Hide resolved
Signed-off-by: Aliaksei Misiukevich <[email protected]>
@nadtech-hub nadtech-hub force-pushed the nadtech/priv-tx-req-helpers branch from c1ab985 to 2ac88db Compare February 7, 2025 12:56
Signed-off-by: Aliaksei Misiukevich <[email protected]>
@nadtech-hub
Copy link
Contributor Author

cool, smol nit

I was struggling to understand usage of this request

@nadtech-hub nadtech-hub requested a review from mattsse February 7, 2025 13:12
Comment on lines +316 to +323
Self {
tx: envelope.encoded_2718().into(),
max_block_number: None,
preferences: PrivateTransactionPreferences {
fast: None,
validity: None,
privacy: None,
},
Copy link
Member

Choose a reason for hiding this comment

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

can we use a new function for this as well, from functions alone are more obfuscated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is empty Bytes allowed?

Copy link
Member

Choose a reason for hiding this comment

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

no but all those fields are pub,

the only thing missing here is this from fn as

impl PrivateTransactionRequest {

pub fn new<T>() -> Self {
Self {
            tx: envelope.encoded_2718().into(),
            max_block_number: None,
            preferences: PrivateTransactionPreferences {
                fast: None,
                validity: None,
                privacy: None,
            },
}

and then the from function would just call PrivateTransactionRequest::new(tx)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes some sense

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.

2 participants