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

Make TxData functions public #23154

Closed
fedekunze opened this issue Jul 2, 2021 · 5 comments
Closed

Make TxData functions public #23154

fedekunze opened this issue Jul 2, 2021 · 5 comments

Comments

@fedekunze
Copy link

fedekunze commented Jul 2, 2021

Rationale

Why should this feature exist?

The existing TxData interface defines its functions as unexported, making it impossible to provide our own implementation of these methods.

// TxData is the underlying data of a transaction.
//
// This is implemented by DynamicFeeTx, LegacyTx and AccessListTx.
type TxData interface {
txType() byte // returns the type ID
copy() TxData // creates a deep copy and initializes all fields
chainID() *big.Int
accessList() AccessList
data() []byte
gas() uint64
gasPrice() *big.Int
gasTipCap() *big.Int
gasFeeCap() *big.Int
value() *big.Int
nonce() uint64
to() *common.Address
rawSignatureValues() (v, r, s *big.Int)
setSignatureValues(chainID, v, r, s *big.Int)
}

What are the use-cases?

Our use case is that we need to implement the TxData implementation as a proto.Any that can support all the concrete implementations of the tx data: LegacyTxType, AccessListTxType and DynamicFeeTxType. We want our TxData interface to embed the TxData interface from geth so that we are able to reduce maintenance.

Implementation

Do you have ideas regarding the implementation of this feature?
Make the TxData functions public.

Are you willing to implement this feature?

Yes

@fedekunze
Copy link
Author

cc @s1na

@s1na
Copy link
Contributor

s1na commented Jul 2, 2021

Hey @fedekunze. I had a quick look and it seems in the lifetime of #21502 these accessor methods started out as public methods but were made private in lightclient@389f039#diff-8fb609018eccff10931645347515b8ecc194992cc4ef9e84ebdea87682dd50c3.

I know in general that the core/types API has been usually kinda stable and it's changed with care which I guess is the reason why these TxData methods were made private. But I can bring this up for discussion and if there was agreement on exporting them make a PR to address it.

@fedekunze
Copy link
Author

Thanks @s1na, here's our PR to add support for proto.Any if you need more context: evmos/ethermint#220

@ligi
Copy link
Member

ligi commented Jul 8, 2021

Can you elaborate why you need to serialize tx.data and not the raw bytes of the transaction that already cares for the transaction type and has a spec (EIP)

@fjl
Copy link
Contributor

fjl commented Jul 8, 2021

We expose TxData for only one reason: making creation of types.Transaction from Go code more convenient. The methods of TxData are an internal implementation detail and will never be stable API. It's not a good idea to send these fields over protocol buffers. If your app needs to send/receive Ethereum transactions over protocol buffers, it should accept them as opaque binary data, which can be parsed by Transaction.UnmarshalBinary.

@s1na s1na removed the status:triage label Jul 8, 2021
@fjl fjl closed this as completed Jul 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants