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

fix mutability object es6 branch #2199

Closed
wants to merge 1 commit into from
Closed

fix mutability object es6 branch #2199

wants to merge 1 commit into from

Conversation

OFRBG
Copy link

@OFRBG OFRBG commented Jan 18, 2019

Fixes #2190
The transaction object can be deep cloned by parsing to a string and back. I do coincide that muting an object by passing the reference, without telling the user, is a bad idea. Using ES6.

@nivida
Copy link
Contributor

nivida commented Jan 21, 2019

I will close this PR because the other PR you created does include these changes.

@nivida nivida closed this Jan 21, 2019
@barakman
Copy link
Contributor

barakman commented Oct 9, 2019

@OFRBG:

I'm sorry, but this doesn't seem to be resolved even on version 1.2.1 (which was released long after this issue had been closed here).

I clearly see that function web3.eth.accounts.signTransaction changes the contents of the object passed to it as the first input parameter.

Specifically, I see that it changes the value of the to field from "" to "0x".

It is clearly visible in your code, at file web3-eth-accounts/src/index.js / line 167:

var transaction = tx;

There is obviously no deep cloning of the input object here, hence the lines that follow change it:

transaction.to = tx.to || '0x';
transaction.data = tx.data || '0x';
transaction.value = tx.value || '0x';
transaction.chainId = utils.numberToHex(tx.chainId);

Thanks

Update:

I am guessing that perhaps the deep-cloning was meant to be done via:

tx = helpers.formatters.inputCallFormatter(tx);
var transaction = tx;

If so, then here is how I have verified that this does not achieve the purpose:

var tx2 = tx;
tx = helpers.formatters.inputCallFormatter(tx);
var transaction = tx;
console.log('before:', tx2.to);
transaction.to = tx.to || '0x';
transaction.data = tx.data || '0x';
transaction.value = tx.value || '0x';
transaction.chainId = utils.numberToHex(tx.chainId);
console.log('after:', tx2.to);

The printout is:

before:
after: 0x

@nivida
Copy link
Contributor

nivida commented Oct 9, 2019

The deep cloning got implemented but we were forced to release a old version of web3 as 1.0 and the refactored lib as 2.0-alpha. Will add this to the back-port list in the stabilization issue asap.

@barakman
Copy link
Contributor

barakman commented Oct 9, 2019

@nivida:
Thank you for the very quick response!
By back-port, do you mean that it will be available to me after I npm install with "web3": "1.2.1" specified in my package.json file?

@nivida
Copy link
Contributor

nivida commented Oct 9, 2019

Yep:)

@barakman
Copy link
Contributor

barakman commented Oct 9, 2019

@nivida:

Looking at branch 1.x, I see:

@gabmontes @nivida gabmontes and nivida Add eth.getChainId method to 1.x (#3113) …
Latest commit 5e8738e 20 hours ago

So how could a fix possibly exist on this branch? (since I posted this less 13 hours ago).
Or else, what exactly am I missing here?
Thanks

@nivida
Copy link
Contributor

nivida commented Oct 9, 2019

It will be available as soon as we have back-ported it. Sorry wasn‘t reading your comment close enough. I‘m currently attending devcon in Osaka and will back-port it as soon as possible. Just follow the release notes and feel free to propose a PR with a generic fix for it.

@barakman
Copy link
Contributor

barakman commented Oct 10, 2019

@nivida:
Done (PR #3119), but please note that if somebody out there has been relying on this function to update the tx input parameter, then their code will subsequently become prone to runtime errors.
So I think that we might be better off merging this PR into the next v1.x release rather than into the current one.
Thank you very much for your help :)

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.

None yet

3 participants