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

eth_sendRawTransaction modified to support ethereum transactions #1227

Merged
merged 81 commits into from
Oct 15, 2019

Conversation

Sriep
Copy link
Contributor

@Sriep Sriep commented Jun 13, 2019

  • I added unit tests for any code that added
  • I updated the CHANGELOG.md
  • All IP is original and not copied from another source
  • I assign all copyright to Loom Network for the code in the pull request

Fix for #1177
Changes to support Ethereum transactions
Requires loomnetwork/go-loom#391
Changes to allow access to auth.Config information to determine Chain id. #1348
Add new feature flag EthTxFeature to activate ethereum transaction functionality.

@Sriep Sriep changed the title eth_sendRawTransaction eth_sendRawTransaction modified to support ethereum transactions Jun 13, 2019
Copy link
Contributor

@enlight enlight left a comment

Choose a reason for hiding this comment

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

I think we can do this without a new tx handler (EthTxHandler), new tx ID (4), nor any protobuf changes, here how:

  1. Change VerifySolidity66Byte to check if len(SignedTx.Signature) == 0 if it is assume this is a raw Ethereum tx and use similar code to VerifyEthereumTransacton to recover the sender address.
  2. Use the eth chain ID instead of native-eth when creating the SignedTx in ethereumToTendermintTx

@Sriep
Copy link
Contributor Author

Sriep commented Jul 4, 2019

Creating a new Tx id was so that vm.handler.go.ProcessTx knew how to unwrap the MessageTx.Data object. Is it an DeployTx, CallTx or rpl-wrapped-Ethereum-Transaction object
Alternatives to a new Tx id for an ethereum-transaction are.

  1. Somewhere, maybe VerifySolidity66Byte the transaction self-modifies itself so that messageTx.Data field is converted into a DeplyTx or CallTx, losing the singing information just keeping the Code/Input data.
  2. In vm.handler.go.ProcessTx if unmarshalling messageTx.Data as a DeplyTx or CallTx fails with an error, we have a stab as rlp.Decoding it. Only returning en error if both attempts fail. I'll code this for the moment.
  3. We could intorduce a flag somewhere to indicate how to unwarp messageTx.Data, but really that is much the same as adding a new Tx Id.

enlight added 22 commits October 3, 2019 12:19
# Conflicts:
#	e2e/eth-5-test.toml
#	eth/query/tx.go
#	rpc/query_server.go
Doesn't quite work yet because the signer on the node seems to have a
different chain ID to the one recovered from the signed eth tx.
# Conflicts:
#	eth/polls/polls_test.go
# Conflicts:
#	features/features.go
Also revert the Ethereum ChainConfig changes since these are no longer
needed in this PR.

And tweak the truffle test a bit.
It was attempting to mint the same ERC721 UID as one of the other tests,
which failed because you can't mint the same token twice.
These tests were using eth_getTransactionCount to obtain the account
nonce, this method now expects to be given an Ethereum address instead
of a DAppChain address. To fix this an account mapping is now created
prior to the each nonce test case and the Ethereum address is used in
calls to eth_getTransactionCount.
This RPC function wasn't reusable in any way, so renamed it to make it
more obvious what it actually does.
So there's no need to bypass EthTxHandler in CheckTx, in fact we want
it to run in CheckTx so it can do the basic validation checks.
Copy link
Contributor Author

@Sriep Sriep left a comment

Choose a reason for hiding this comment

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

One suggestion, Looks ok to merge.

@enlight enlight merged commit d36e3dc into master Oct 15, 2019
@enlight enlight deleted the issue1177 branch February 27, 2020 07:46
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.

3 participants