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

Ethereum transaction refactoring #624

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions pkg/metamask/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -163,14 +163,14 @@ func (s RPCService) Eth_EstimateGas(req estimateGasRequest) (string, error) {
}
}

txKind, err := state.GuessEthereumTransactionKind(data)
txKind, err := proto.GuessEthereumTransactionKind(data)
if err != nil {
return "", errors.Errorf("failed to guess ethereum tx kind, %v", err)
}
switch txKind {
case state.EthereumTransferWavesKind:
case proto.EthereumTransferWavesKind:
return fmt.Sprintf("%d", proto.MinFee), nil
case state.EthereumTransferAssetsKind:
case proto.EthereumTransferAssetsKind:
fee := proto.MinFee
assetID := (*proto.AssetID)(req.To)

Expand All @@ -182,7 +182,7 @@ func (s RPCService) Eth_EstimateGas(req estimateGasRequest) (string, error) {
fee += proto.MinFeeScriptedAsset
}
return fmt.Sprintf("%d", fee), nil
case state.EthereumInvokeKind:
case proto.EthereumInvokeKind:
fee := proto.MinFeeInvokeScript

scriptAddr, err := req.To.ToWavesAddress(s.nodeRPCApp.Scheme)
Expand All @@ -201,7 +201,7 @@ func (s RPCService) Eth_EstimateGas(req estimateGasRequest) (string, error) {
if err != nil {
return "", err
}
decodedData, err := db.ParseCallDataRide(data)
decodedData, err := db.ParseCallData(data)
if err != nil {
return "", errors.Errorf("failed to parse ethereum data, %v", err)
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/proto/eth_access_list_tx_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ func TestEthereumAccessListTxCanonical(t *testing.T) {
require.NoError(t, err)
return data
}
const expectedCanonical = "0x01f8630103018261a894b94f5374fce5edbc8e2a8697c15331677e6ebf0b0a825544c001a0c9519f4f2b30335884581971573fadf60c6204f59a911df35ee8a540456b2660a032f1e8e2c5dd761f9e4f88f41c8310aeaba26a8bfcdacfedfa12ec3862d37521"
const expectedCanonical = "0x01f8650103018261a894b94f5374fce5edbc8e2a8697c15331677e6ebf0b0a84bdc01110c001a0c9519f4f2b30335884581971573fadf60c6204f59a911df35ee8a540456b2660a032f1e8e2c5dd761f9e4f88f41c8310aeaba26a8bfcdacfedfa12ec3862d37521"
var (
expectedCanonicalBytes = mustDecodeFomHexString(expectedCanonical)
testAddrBytes = mustDecodeFomHexString("0xb94f5374fce5edbc8e2a8697c15331677e6ebf0b")
Expand All @@ -34,7 +34,7 @@ func TestEthereumAccessListTxCanonical(t *testing.T) {
Value: big.NewInt(10),
Gas: 25000,
GasPrice: big.NewInt(1),
Data: mustDecodeFomHexString("5544"),
Data: mustDecodeFomHexString("0xbdc01110"),
V: &v,
R: &r,
S: &s,
Expand Down
6 changes: 3 additions & 3 deletions pkg/proto/eth_signer.go
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ func (s londonSigner) SenderPK(tx *EthereumTransaction) (*EthereumPublicKey, err
// DynamicFee txs are defined to use 0 and 1 as their recovery
// id, add 27 to become equivalent to unprotected Homestead signatures.
V = new(big.Int).Add(V, big.NewInt(27))
if tx.ChainId().Cmp(s.chainId) != 0 {
if tx.inner.chainID().Cmp(s.chainId) != 0 {
return nil, ErrInvalidChainId
}
return recoverEthereumPubKey(s.Hash(tx), R, S, V, true)
Expand Down Expand Up @@ -262,7 +262,7 @@ func (s eip2930Signer) SenderPK(tx *EthereumTransaction) (*EthereumPublicKey, er
// AL txs are defined to use 0 and 1 as their recovery
// id, add 27 to become equivalent to unprotected Homestead signatures.
V = new(big.Int).Add(V, big.NewInt(27))
if tx.ChainId().Cmp(s.chainId) != 0 {
if tx.inner.chainID().Cmp(s.chainId) != 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Why did you change tx.ChainId() to tx.inner.chainID()? What is the reason?

return nil, ErrInvalidChainId
}
return recoverEthereumPubKey(s.Hash(tx), R, S, V, true)
Expand Down Expand Up @@ -342,7 +342,7 @@ func (s eip155Signer) SenderPK(tx *EthereumTransaction) (*EthereumPublicKey, err
if !tx.Protected() {
return HomesteadSigner{}.SenderPK(tx)
}
if tx.ChainId().Cmp(s.chainId) != 0 {
if tx.inner.chainID().Cmp(s.chainId) != 0 {
Copy link
Member

Choose a reason for hiding this comment

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

The same as above.

return nil, ErrInvalidChainId
}
V, R, S := tx.RawSignatureValues()
Expand Down
180 changes: 148 additions & 32 deletions pkg/proto/eth_transaction.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,12 @@ const (
EthereumDynamicFeeTxType
)

const (
EthereumTransferWavesKind = iota + 1
EthereumTransferAssetsKind
EthereumInvokeKind
)

const (
EthereumTransferMinFee uint64 = 100_000
EthereumScriptedAssetMinFee uint64 = 400_000
Expand Down Expand Up @@ -91,7 +97,7 @@ type EthereumTxData interface {

type EthereumTransactionKind interface {
String() string
DecodedData() *ethabi.DecodedCallData
DecodedData() (*ethabi.DecodedCallData, error)
}

type EthereumTransferWavesTxKind struct {
Expand All @@ -101,8 +107,8 @@ func NewEthereumTransferWavesTxKind() *EthereumTransferWavesTxKind {
return &EthereumTransferWavesTxKind{}
}

func (tx *EthereumTransferWavesTxKind) DecodedData() *ethabi.DecodedCallData {
return nil
func (tx *EthereumTransferWavesTxKind) DecodedData() (*ethabi.DecodedCallData, error) {
return nil, errors.New("transfer waves ethereum tx kind does not have decoded call data")
}

func (tx *EthereumTransferWavesTxKind) String() string {
Expand All @@ -112,42 +118,57 @@ func (tx *EthereumTransferWavesTxKind) String() string {
type EthereumTransferAssetsErc20TxKind struct {
decodedData ethabi.DecodedCallData
Arguments ethabi.ERC20TransferArguments
Asset OptionalAsset
OptAsset *OptionalAsset
}

func NewEthereumTransferAssetsErc20TxKind(decodedData ethabi.DecodedCallData, asset OptionalAsset, arguments ethabi.ERC20TransferArguments) *EthereumTransferAssetsErc20TxKind {
return &EthereumTransferAssetsErc20TxKind{Asset: asset, decodedData: decodedData, Arguments: arguments}
func NewEthereumTransferAssetsErc20TxKind(decodedData ethabi.DecodedCallData, asset *OptionalAsset, arguments ethabi.ERC20TransferArguments) *EthereumTransferAssetsErc20TxKind {
return &EthereumTransferAssetsErc20TxKind{OptAsset: asset, decodedData: decodedData, Arguments: arguments}
}

func (tx *EthereumTransferAssetsErc20TxKind) DecodedData() *ethabi.DecodedCallData {
return &tx.decodedData
func (tx *EthereumTransferAssetsErc20TxKind) DecodedData() (*ethabi.DecodedCallData, error) {
return &tx.decodedData, nil
}
func (tx *EthereumTransferAssetsErc20TxKind) Asset() (*OptionalAsset, error) {
if tx.OptAsset == nil {
return nil, errors.New("asset field of ethereum transfer assets tx is empty")
}
return tx.OptAsset, nil
}

func (tx *EthereumTransferAssetsErc20TxKind) String() string {
return "EthereumTransferAssetsErc20TxKind"
}

type EthereumInvokeScriptTxKind struct {
decodedData ethabi.DecodedCallData
DecodedCallData *ethabi.DecodedCallData
}

func NewEthereumInvokeScriptTxKind(decodedData ethabi.DecodedCallData) *EthereumInvokeScriptTxKind {
return &EthereumInvokeScriptTxKind{decodedData: decodedData}
func NewEthereumInvokeScriptTxKind(decodedData *ethabi.DecodedCallData) *EthereumInvokeScriptTxKind {
return &EthereumInvokeScriptTxKind{DecodedCallData: decodedData}
}

func (tx *EthereumInvokeScriptTxKind) DecodedData() *ethabi.DecodedCallData {
return &tx.decodedData
func (tx *EthereumInvokeScriptTxKind) DecodedData() (*ethabi.DecodedCallData, error) {
if tx.DecodedCallData == nil {
return nil, errors.New("ethereum invoke script tx has empty decoded data")
}
return tx.DecodedCallData, nil
}

func (tx *EthereumInvokeScriptTxKind) String() string {
return "EthereumInvokeScriptTxKind"
}

type EthereumTransaction struct {
inner EthereumTxData
innerBinarySize int
senderPK atomic.Value // *EthereumPublicKey
// Ethereum representation
senderPK atomic.Value // *EthereumPublicKey
inner EthereumTxData //

// Waves representation
TxKind EthereumTransactionKind
Comment on lines +162 to 167
Copy link
Member

Choose a reason for hiding this comment

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

Use different naming for Ethereum representation fields and Waves representation fields. I think it would be better if you create special struct for valid waves representation.

value int64
chainID int64
Copy link
Member

Choose a reason for hiding this comment

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

Use proto.Scheme type instead of int64. And for WAVES network correct naming is sheme, not chainID.

gasPrice uint64
innerBinarySize int
Copy link
Member

Choose a reason for hiding this comment

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

This is not Waves representation. It's a utility info

ID *crypto.Digest
}

Expand All @@ -159,6 +180,9 @@ func NewEthereumTransaction(inner EthereumTxData, txKind EthereumTransactionKind
TxKind: txKind,
ID: id,
}
res := new(big.Int).Div(tx.inner.value(), big.NewInt(int64(DiffEthWaves)))
tx.value = res.Int64()

Comment on lines +183 to +185
Copy link
Member

Choose a reason for hiding this comment

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

You should handle it res doesn't fit into int64 value even in tests.

tx.threadSafeSetSenderPK(senderPK)
return tx
}
Expand Down Expand Up @@ -215,7 +239,7 @@ func (tx *EthereumTransaction) Verify() (*EthereumPublicKey, error) {
if senderPK := tx.threadSafeGetSenderPK(); senderPK != nil {
return senderPK, nil
}
signer := MakeEthereumSigner(tx.ChainId())
signer := MakeEthereumSigner(tx.inner.chainID())
Copy link
Member

Choose a reason for hiding this comment

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

Use tx.inner.chainID() in all places and remove tx.ChainID() method. Or use tx.ChainId() and don't use tx.inner.chainID() anywhere.

senderPK, err := signer.SenderPK(tx)
if err != nil {
return nil, errors.Wrap(err, "failed to verify EthereumTransaction")
Expand All @@ -228,9 +252,9 @@ func (tx *EthereumTransaction) Verify() (*EthereumPublicKey, error) {
// This method doesn't include signature verification. Use Verify method for signature verification
func (tx *EthereumTransaction) Validate(scheme Scheme) (Transaction, error) {
// same chainID
if tx.ChainId().Cmp(big.NewInt(int64(scheme))) != 0 {
if tx.ChainId() != int64(scheme) {
// TODO: introduce new error type for scheme validation
txChainID := tx.ChainId().Uint64()
txChainID := tx.ChainId()
return nil, errs.NewTxValidationError(fmt.Sprintf(
"Address belongs to another network: expected: %d(%c), actual: %d(%c)",
scheme, scheme,
Expand All @@ -250,6 +274,7 @@ func (tx *EthereumTransaction) Validate(scheme Scheme) (Transaction, error) {
return nil, errs.NewFeeValidation("insufficient fee")
}
// too many waves (this check doesn't exist in scala)
// TODO I'm not sure this is should be checked for all eth tx kinds. Only for transfer waves kind
Copy link
Member

Choose a reason for hiding this comment

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

It's valid. Negative wavelets amount is invalid. It must be zero or greater.

wavelets, err := EthereumWeiToWavelet(tx.Value())
if err != nil {
return nil, errs.NewFeeValidation(err.Error())
Expand All @@ -259,15 +284,15 @@ func (tx *EthereumTransaction) Validate(scheme Scheme) (Transaction, error) {
return nil, errs.NewNonPositiveAmount(wavelets, "waves")
}
// a cancel transaction: value == 0 && data == 0x
if tx.Value().Cmp(big0) == 0 && len(tx.Data()) == 0 {
if tx.Value() == 0 && len(tx.Data()) == 0 {
return nil, errs.NewTxValidationError("Transaction cancellation is not supported")
}
// either data or value field is set
if tx.Value().Cmp(big0) != 0 && len(tx.Data()) != 0 {
if tx.Value() != 0 && len(tx.Data()) != 0 {
return nil, errs.NewTxValidationError("Transaction should have either data or value")
}
// gasPrice == 10GWei
if tx.GasPrice().Cmp(new(big.Int).SetUint64(EthereumGasPrice)) != 0 {
if tx.GasPrice() != EthereumGasPrice {
return nil, errs.NewTxValidationError("Gas price must be 10 Gwei")
}
// deny a contract creation transaction (this check doesn't exist in scala)
Expand Down Expand Up @@ -375,8 +400,8 @@ func (tx *EthereumTransaction) EthereumTxType() EthereumTxType {
// ChainId returns the EIP155 chain ID of the transaction. The return value will always be
// non-nil. For legacy transactions which are not replay-protected, the return value is
// zero.
func (tx *EthereumTransaction) ChainId() *big.Int {
return tx.inner.chainID()
func (tx *EthereumTransaction) ChainId() int64 {
return tx.chainID
}
Comment on lines +403 to 405
Copy link
Member

Choose a reason for hiding this comment

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

You should change return type to proto.Scheme. And I think it would be better if you rename this method to Scheme().


// Data returns the input data of the transaction.
Expand All @@ -389,16 +414,10 @@ func (tx *EthereumTransaction) AccessList() EthereumAccessList { return tx.inner
func (tx *EthereumTransaction) Gas() uint64 { return tx.inner.gas() }

// GasPrice returns the gas price of the transaction.
func (tx *EthereumTransaction) GasPrice() *big.Int { return copyBigInt(tx.inner.gasPrice()) }

// GasTipCap returns the gasTipCap per gas of the transaction.
func (tx *EthereumTransaction) GasTipCap() *big.Int { return copyBigInt(tx.inner.gasTipCap()) }

// GasFeeCap returns the fee cap per gas of the transaction.
func (tx *EthereumTransaction) GasFeeCap() *big.Int { return copyBigInt(tx.inner.gasFeeCap()) }
func (tx *EthereumTransaction) GasPrice() uint64 { return tx.gasPrice }

// Value returns the ether amount of the transaction.
func (tx *EthereumTransaction) Value() *big.Int { return copyBigInt(tx.inner.value()) }
func (tx *EthereumTransaction) Value() int64 { return tx.value }
Comment on lines -401 to +420
Copy link
Member

Choose a reason for hiding this comment

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

Comment says that Value returns the ether amount of the transaction.. You return wavelets


// Nonce returns the sender account nonce of the transaction.
func (tx *EthereumTransaction) Nonce() uint64 { return tx.inner.nonce() }
Expand Down Expand Up @@ -458,6 +477,98 @@ func (tx *EthereumTransaction) RawSignatureValues() (v, r, s *big.Int) {
return tx.inner.rawSignatureValues()
}

func validateEthereumTx(tx *EthereumTransaction) error {
switch tx.TxKind.(type) {
case *EthereumTransferWavesTxKind:
res := new(big.Int).Div(tx.inner.value(), big.NewInt(int64(DiffEthWaves)))
if ok := res.IsInt64(); !ok {
return errors.Errorf("failed to convert amount from ethreum transaction (big int) to int64. value is %d", tx.Value())
}
Comment on lines +483 to +486
Copy link
Member

Choose a reason for hiding this comment

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

You can easily use previous version for of EthereumWeiToWavelet function for conversion.

tx.value = res.Int64()
return nil
case *EthereumTransferAssetsErc20TxKind:
return nil
case *EthereumInvokeScriptTxKind:
return nil
Comment on lines +489 to +492
Copy link
Member

Choose a reason for hiding this comment

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

Merge this cases or rewrite whole switch to if...else.

default:
return errors.New("failed to check ethereum transaction, wrong kind of tx")
}
}

func GuessEthereumTransactionKind(data []byte) (int64, error) {
if len(data) == 0 {
return EthereumTransferWavesKind, nil
}

selectorBytes := data
Copy link
Member

Choose a reason for hiding this comment

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

This is unnecessary variable

if len(data) < ethabi.SelectorSize {
return 0, errors.Errorf("length of data from ethereum transaction is less than %d", ethabi.SelectorSize)
}
selector, err := ethabi.NewSelectorFromBytes(selectorBytes[:ethabi.SelectorSize])
Copy link
Member

Choose a reason for hiding this comment

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

data[:ethabi.SelectorSize] does the same. Maybe you meant selectorBytes := data[:ethabi.SelectorSize]. Correct me if I'm wrong

if err != nil {
return 0, errors.Wrap(err, "failed to guess ethereum transaction kind")
}

if ethabi.IsERC20TransferSelector(selector) {
return EthereumTransferAssetsKind, nil
}

return EthereumInvokeKind, nil
}

// note: in this method not all of the fields are filled
func GetEthereumTransactionKind(ethTx EthereumTransaction) (EthereumTransactionKind, error) {
txKind, err := GuessEthereumTransactionKind(ethTx.Data())
if err != nil {
return nil, errors.Wrap(err, "failed to guess ethereum tx kind")
}

switch txKind {
case EthereumTransferWavesKind:
return NewEthereumTransferWavesTxKind(), nil
case EthereumTransferAssetsKind:
db := ethabi.NewErc20MethodsMap()
decodedData, err := db.ParseCallData(ethTx.Data())
if err != nil {
return nil, errors.Errorf("failed to parse ethereum data")
}
if len(decodedData.Inputs) != ethabi.NumberOfERC20TransferArguments {
Copy link
Member

Choose a reason for hiding this comment

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

This check is unnecessary. ethabi.GetERC20TransferArguments does the same check inside.

return nil, errors.Errorf("the number of arguments of erc20 function is %d, but expected it to be %d", len(decodedData.Inputs), ethabi.NumberOfERC20TransferArguments)
}

erc20Arguments, err := ethabi.GetERC20TransferArguments(decodedData)
if err != nil {
return nil, errors.Wrap(err, "failed to get erc20 arguments from decoded data")
}
return NewEthereumTransferAssetsErc20TxKind(*decodedData, nil, erc20Arguments), nil
case EthereumInvokeKind:
return NewEthereumInvokeScriptTxKind(nil), nil
default:
return nil, errors.New("unexpected ethereum tx kind")
}
}

func (tx *EthereumTransaction) initTransactionFields() error {
if ok := tx.inner.chainID().IsInt64(); !ok {
return errors.Errorf("failed to recognize chainID of ethereum transaction: over int64")
}
tx.chainID = tx.inner.chainID().Int64()
if ok := tx.inner.gasPrice().IsUint64(); !ok {
return errors.Errorf("failed to recognize GasPrice of ethereum transaction: over uint64")
}
Comment on lines +555 to +558
Copy link
Member

Choose a reason for hiding this comment

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

You should check that chainID fits in proto.Scheme type.

tx.gasPrice = tx.inner.gasPrice().Uint64()
var err error
tx.TxKind, err = GetEthereumTransactionKind(*tx)
if err != nil {
return errors.Errorf("failed to guess ethereum transaction kind, %v", err)
}
err = validateEthereumTx(tx)
if err != nil {
return errors.Errorf("validation of ethereum transaction after initialization failed , %v", err)
}
return nil
}

// DecodeCanonical decodes the canonical binary encoding of transactions.
// It supports legacy RLP transactions and EIP2718 typed transactions.
func (tx *EthereumTransaction) DecodeCanonical(canonicalData []byte) error {
Expand Down Expand Up @@ -488,6 +599,11 @@ func (tx *EthereumTransaction) DecodeCanonical(canonicalData []byte) error {
tx.inner = inner
}
tx.innerBinarySize = len(canonicalData)

err := tx.initTransactionFields()
if err != nil {
return errors.Wrap(err, "failed to initialize waves representation fields of ethereum transaction")
}
return nil
}

Expand Down
Loading