search

paradigmxyz / reth

Modular, contributor-friendly and blazing-fast implementation of the Ethereum protocol, in Rust
https://reth.rs/
Apache License 2.0
3.78k stars 1.02k forks source link

Replace ethsigner trait with alloy-signer #7490

Open mattsse opened 5 months ago

mattsse commented 5 months ago

Describe the feature

we now have alloy-signer as dependency, and the EthSigner trait:

https://github.com/paradigmxyz/reth/blob/b89af430e2f45f8a6e53776a6e5150c1b21458c6/crates/rpc/rpc/src/eth/signer.rs#L19-L19

can now be replaced with alloy signer traits (NetworkSigner)

we can also replace all the typedtransaction types: https://github.com/paradigmxyz/reth/blob/b89af430e2f45f8a6e53776a6e5150c1b21458c6/crates/rpc/rpc-types/src/eth/transaction/typed.rs#L18-L18

in favor of https://github.com/alloy-rs/alloy/blob/098907b76208f786f3070e23c76428d692e94dc3/crates/network/src/transaction/builder.rs#L48-L48 which is implemented for TransactionRequest

https://github.com/alloy-rs/alloy/blob/098907b76208f786f3070e23c76428d692e94dc3/crates/network/src/transaction/builder.rs#L212-L216

cc @allnil

Additional context

No response

justcode740 commented 5 months ago

will take a look

mattsse commented 5 months ago

thanks @justcode740, you're already assigned to a few other issues. I'll leave this open in case anyone else wants to take this or until you've wrapped up the others :)

allnil commented 5 months ago

will look at it

onbjerg commented 4 months ago

Hey @allnil, what's the status of this?

allnil commented 4 months ago

@onbjerg hey! going to finish it this week

allnil commented 4 months ago

in progress

Rjected commented 3 months ago

@allnil update on this?

kamuik16 commented 1 month ago

Describe the feature

we now have alloy-signer as dependency, and the EthSigner trait:

https://github.com/paradigmxyz/reth/blob/b89af430e2f45f8a6e53776a6e5150c1b21458c6/crates/rpc/rpc/src/eth/signer.rs#L19-L19

can now be replaced with alloy signer traits (NetworkSigner)

we can also replace all the typedtransaction types:

https://github.com/paradigmxyz/reth/blob/b89af430e2f45f8a6e53776a6e5150c1b21458c6/crates/rpc/rpc-types/src/eth/transaction/typed.rs#L18-L18

in favor of https://github.com/alloy-rs/alloy/blob/098907b76208f786f3070e23c76428d692e94dc3/crates/network/src/transaction/builder.rs#L48-L48 which is implemented for TransactionRequest

https://github.com/alloy-rs/alloy/blob/098907b76208f786f3070e23c76428d692e94dc3/crates/network/src/transaction/builder.rs#L212-L216

cc @allnil

Additional context

No response

Hey @mattsse! Is this issue open to work?

mattsse commented 1 month ago

hey @kamuik16 are you still interested in this?

allnil commented 1 month ago

yeh, guys, sorry, I don't have capacity to finish it, pretty big issue and I was quite overloaded

mattsse commented 1 month ago

all good, the draft should be useful for anyone that'll pick this up, ty @allnil

mvares commented 1 month ago

@mattsse that’s interesting. I’m finishing my last issue and go for this

mvares commented 1 month ago

@mattsse, will all components that implement the trait EthSigner possibly be changed?

mattsse commented 1 month ago

I think we can keep the trait but hide the signing internally like:

https://github.com/alloy-rs/alloy/blob/c3ccf7e2c5f9720fa6c193a9ea918aabf88aa6a2/crates/network/src/transaction/signer.rs#L51-L60

similar to:

https://github.com/paradigmxyz/reth/pull/8614/files#diff-fc791d9a535c846c23e379d9e1656f7c2f5974e06c91da0ce8d89e0322b1696bR1063-R1072

I'm fine with the rlp roundtrip

mvares commented 1 month ago

I think we can keep the trait but hide the signing internally like:

https://github.com/alloy-rs/alloy/blob/c3ccf7e2c5f9720fa6c193a9ea918aabf88aa6a2/crates/network/src/transaction/signer.rs#L51-L60

similar to:

https://github.com/paradigmxyz/reth/pull/8614/files#diff-fc791d9a535c846c23e379d9e1656f7c2f5974e06c91da0ce8d89e0322b1696bR1063-R1072

I'm fine with the rlp roundtrip

well, what I was wondering:

continue with trait EthSigner and only assigns it with Signer from alloy-signer

cc @mattsse

mvares commented 1 month ago

I think we can keep the trait but hide the signing internally like:

https://github.com/alloy-rs/alloy/blob/c3ccf7e2c5f9720fa6c193a9ea918aabf88aa6a2/crates/network/src/transaction/signer.rs#L51-L60

similar to:

https://github.com/paradigmxyz/reth/pull/8614/files#diff-fc791d9a535c846c23e379d9e1656f7c2f5974e06c91da0ce8d89e0322b1696bR1063-R1072

I'm fine with the rlp roundtrip

btw, this makes sense if we keep the trait, maybe could be benefincial for we...