Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

There is no need to check if the recipient of a transaction is a contract or not #119

Closed
wants to merge 1 commit into from

Conversation

bernardpeh
Copy link

Everytime ganache sees a "data" field, it assumes that it is trying to call a contract function and that the "to" field must be a contract. This should not be the case.

It should be perfectly fine to do this:

web3.eth.sendTransaction({from:web3.eth.accounts[0],to:web3.eth.accounts[1],value:web3.toWei(0.02,“ether”),data:web3.toHex(“hellow world”)})

This above snippet below is working in testnet but not in ganache.

@guilhermepozo
Copy link

I'm trying to send a transaction with some data to an account (not a contract) and i'm receiving:

Attempting to run transaction which calls a contract function, but recipient address 0x6e2e3accbe4b1320107118147a7cb6f5b689ea2f is not a contract address.

Ethereum "blocks/disable" transactions (Acc2Acc) with data?

@benjamincburns
Copy link
Contributor

@bernardpeh Thanks for this.

I added this check to be helpful in the case where the caller is attempting to call a contract but the contract address is incorrect, but you're correct that in doing so I inadvertently prevented users from sending transactions with arbitrary data.

IIRC, it's possible to check whether the data field is formatted a function call. In the case when it is formatted as such, I'd like to keep this check in place. Could you update your PR to handle this?

Also it seems that you didn't run tests before submitting, as this change caused a test to fail. Please be sure to check your changes with npm test and update tests as necessary.

@benjamincburns
Copy link
Contributor

Going to keep #117 around but since I haven't heard back from @bernardpeh on this one and since it causes numerous test failures I'm going to close in hopes that a better submission comes along.

@bernardpeh if you want to update this to make tests pass please feel free to submit a new PR.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants