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

internal/ethapi: support "input" in transaction args #15640

Merged
merged 1 commit into from
Dec 18, 2017

Conversation

fjl
Copy link
Contributor

@fjl fjl commented Dec 9, 2017

The tx data field is called "input" in returned objects and "data" in
argument objects. Make it so "input" can be used, but bail if both are
set.

Fixes #15628

@fjl fjl force-pushed the ethapi-tx-input branch from c201473 to 83ed325 Compare December 9, 2017 23:05
@fjl fjl changed the title internal/ethapi: support "input" in transaction objects internal/ethapi: support "input" in transaction args Dec 9, 2017
@arrivets
Copy link

There is still a problem that raw transactions are decoded to core.types.Transaction which uses the 'input' field name for payload.

from Geth/internal/ethapi/api.go:

func (s *PublicTransactionPoolAPI) SendRawTransaction(ctx context.Context, encodedTx hexutil.Bytes) (string, error) {
	tx := new(types.Transaction)
	if err := rlp.DecodeBytes(encodedTx, tx); err != nil {
		return "", err
	}

The current javascript tooling to construct and sign transactions uses the 'data' field:

ex web3-eth-accounts:

Accounts.prototype.signTransaction = function signTransaction(tx, privateKey, callback) {
    var _this = this;

    function signed (tx) {

        if (!tx.gas && !tx.gasLimit) {
            throw new Error('"gas" is missing');
        }

        var transaction = {
            nonce: utils.numberToHex(tx.nonce),
            to: tx.to ? helpers.formatters.inputAddressFormatter(tx.to) : '0x',
            data: tx.data || '0x',
            value: tx.value ? utils.numberToHex(tx.value) : "0x",
            gas: utils.numberToHex(tx.gasLimit || tx.gas),
            gasPrice: utils.numberToHex(tx.gasPrice),
            chainId: utils.numberToHex(tx.chainId)
        };

Am I missing something?

@holiman
Copy link
Contributor

holiman commented Dec 12, 2017

@arrivets Yes, I think you are.. For the rlp-decoding, it doesn't matter what field is used internally. This PR would still accept current tooliing using data in the supplied json object.

@holiman
Copy link
Contributor

holiman commented Dec 12, 2017

I think this PR makes sense, but instead of failing if both are non-nil, it could check if they are equal first.

@arrivets
Copy link

ah yes of course.. thanks @holiman

@@ -1093,14 +1094,23 @@ func (args *SendTxArgs) setDefaults(ctx context.Context, b Backend) error {
}
args.Nonce = (*hexutil.Uint64)(&nonce)
}
if args.Input != nil && args.Data != nil {
return errors.New(`"data" and "input" are mutually exclusive`)

Choose a reason for hiding this comment

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

The error message is a bit obfuscated. It's not entirely obvious from the error message that it will fail if the user supplies both. When two entities are mutually exclusive, it does not require that one be nil.

Perhaps it makes sense to have the error message something on the lines of: "Please supply either data or input."

@fjl
Copy link
Contributor Author

fjl commented Dec 12, 2017

PTAL

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM (have not yet tested, only looked at the code)

The tx data field is called "input" in returned objects and "data" in
argument objects. Make it so "input" can be used in args, but bail if
both "input" and "data" are set.

Fixes ethereum#15628
@fjl
Copy link
Contributor Author

fjl commented Dec 14, 2017

Rebased again to remove lint failure

@fjl fjl merged commit 8c33ac1 into ethereum:master Dec 18, 2017
@karalabe karalabe added this to the 1.8.0 milestone Dec 20, 2017
b00ris pushed a commit to b00ris/go-ethereum that referenced this pull request Jan 19, 2018
The tx data field is called "input" in returned objects and "data" in
argument objects. Make it so "input" can be used, but bail if both
are set.
mariameda pushed a commit to NiluPlatform/go-nilu that referenced this pull request Aug 23, 2018
The tx data field is called "input" in returned objects and "data" in
argument objects. Make it so "input" can be used, but bail if both
are set.
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.

5 participants