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: Add getters into TransactionResponse and update implementations #1328

Merged

Conversation

garwahl
Copy link
Contributor

@garwahl garwahl commented Sep 22, 2024

Closes #1297

Adds getters into the TransactionResponse trait and updates implementations accordingly.

/// Input data
#[doc(alias = "calldata")]
fn input(&self) -> &Bytes;

/// Transaction signature
fn signature(&self) -> Option<Signature>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@emhane Signature comes from alloy-rpc-types-eth which can't be imported network-primitives/Cargo.toml due to a cyclic dependency since rpc-types-eth/Cargo.toml already pulls in network-primitives.

Do you have a suggestion here?

Copy link
Member

Choose a reason for hiding this comment

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

probably best to add signature as associated type to the trait

/// Input data
#[doc(alias = "calldata")]
fn input(&self) -> &Bytes;

/// Transaction signature
fn signature(&self) -> Option<Signature>;
Copy link
Member

Choose a reason for hiding this comment

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

probably best to add signature as associated type to the trait

@garwahl garwahl marked this pull request as ready for review September 23, 2024 11:13
@garwahl
Copy link
Contributor Author

garwahl commented Sep 23, 2024

@emhane I've updated this to use an associated type, feel free to merge if LGTY or happy to wait for a +1 from another reviewer

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.

wdyt @klkvr

@@ -63,10 +64,24 @@ pub trait ReceiptResponse {

/// Transaction JSON-RPC response.
pub trait TransactionResponse {
Copy link
Member

Choose a reason for hiding this comment

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

Is this now just Transaction + additional functions to get the block context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, although I'm not entirely across the context. It looks similar to how ReceiptResponse is a projection of the fields on a TransactionReceipt. Happy to be guided by owners on this

Copy link
Member

@klkvr klkvr left a comment

Choose a reason for hiding this comment

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

lgtm, this is now very similar to Transaction trait, we can dedup those in a follow-up

@mattsse
Copy link
Member

mattsse commented Sep 23, 2024

another thing we can do is restrict the signature to EncodeableSignature perhaps

@garwahl
Copy link
Contributor Author

garwahl commented Sep 23, 2024

Thanks @klkvr @mattsse @emhane! Can one of you kick off a merge if LGTY, happy to address in a follow up.

another thing we can do is restrict the signature to EncodeableSignature perhaps

@mattsse alloy_rpc_types_eth::transaction::Signature doesn't implement EncodeableSignature. If this is something we want perhaps we can do it in a follow up + enforcement?

@mattsse
Copy link
Member

mattsse commented Sep 23, 2024

imo, that would make sense

but ideally we could phase this out entirely

alloy_rpc_types_eth::transaction::Signature

@mattsse mattsse merged commit 789aee8 into alloy-rs:main Sep 23, 2024
26 checks passed
@emhane
Copy link
Member

emhane commented Sep 23, 2024

lgtm, this is now very similar to Transaction trait, we can dedup those in a follow-up

probably will be nicest if we have the consensus Transaction trait as super trait of the rpc TransactionResponse trait, because the way the transaction response is built is: Transaction type + signature + singer address + block context. hence, makes sense if TransactionResponse is an extension of core Transaction.

ref #1316

@klkvr @mattsse

@klkvr
Copy link
Member

klkvr commented Sep 23, 2024

probably will be nicest if we have the consensus Transaction trait as super trait of the rpc TransactionResponse trait

agree, was thinking about this as well

lwedge99 pushed a commit to sentioxyz/alloy that referenced this pull request Oct 8, 2024
…alloy-rs#1328)

* init

* wip: fix signature import

* associated type

---------

Co-authored-by: garwah <garwah@garwah>
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.

[Feature] Add getters to alloy_network_primitives::TransactionResponse
4 participants