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

fix: remove Borrow impl for RPC receipt #1721

Merged
merged 1 commit into from
Dec 2, 2024
Merged

fix: remove Borrow impl for RPC receipt #1721

merged 1 commit into from
Dec 2, 2024

Conversation

klkvr
Copy link
Member

@klkvr klkvr commented Nov 30, 2024

Motivation

Based on #1719

I've added this impl to support a more generic AnyReceiptEnvelope

impl<T> Borrow<alloy_primitives::Log<T>> for Log<T> {
fn borrow(&self) -> &alloy_primitives::Log<T> {
&self.inner
}
}

However, it is in fact incorrect as Borrow shouldn't be implemented in this case

Solution

This PR reverts changes including this impl and changes AnyReceiptEnvelope back to always holding a Receipt<T>

If we want to still support AnyReceiptEnvelope being generic over arbitrary receipt types, we'd need to either add AsRef<Self> for alloy_primitives::Log and use it instead or introduce a separate Log trait to replace such bounds with:

impl<T> Receipt<T>
where
T: Borrow<Log>,

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

@@ -16,23 +17,23 @@ use alloy_rlp::{Decodable, Encodable};
#[derive(Clone, Debug, PartialEq, Eq)]
#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
#[doc(alias = "AnyTransactionReceiptEnvelope", alias = "AnyTxReceiptEnvelope")]
pub struct AnyReceiptEnvelope<T = Receipt<Log>> {
pub struct AnyReceiptEnvelope<T = Log> {
Copy link
Member

Choose a reason for hiding this comment

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

this could potentially be problematic for use cases where the any network should support different receipt types, for example op.

but this doesn't change the defaults and is fine imo.

we should also add

AsRef for alloy_primitives::Log

to alloy_primitives

Base automatically changed from klkvr/receipts to main December 2, 2024 11:23
@klkvr klkvr force-pushed the klkvr/rm-borrow-impl branch from ad8d400 to 6b5cfa7 Compare December 2, 2024 11:24
@klkvr klkvr force-pushed the klkvr/rm-borrow-impl branch from 6b5cfa7 to a59d90e Compare December 2, 2024 11:25
@klkvr klkvr enabled auto-merge (squash) December 2, 2024 11:25
@klkvr klkvr merged commit 7b4db9e into main Dec 2, 2024
26 checks passed
@klkvr klkvr deleted the klkvr/rm-borrow-impl branch December 2, 2024 11:32
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