Skip to content
This repository has been archived by the owner on May 24, 2022. It is now read-only.

Add gas estimation to the postTransaction RPC #279

Merged
merged 15 commits into from
Dec 11, 2018

Conversation

Tbaut
Copy link
Collaborator

@Tbaut Tbaut commented Dec 6, 2018

@Tbaut Tbaut changed the title [WIP] Support exact amount for account to account transactions Support exact amount for account to account transactions Dec 7, 2018
@Tbaut Tbaut changed the title Support exact amount for account to account transactions [WIP] Support exact amount for account to account transactions Dec 7, 2018
@Tbaut
Copy link
Collaborator Author

Tbaut commented Dec 7, 2018

Should be good to go for ETH transactions, albeit dodgy code, let me know what you think.
I was able to send the exact amount of ETH and leave an account to 0, I tested and didn't break sending tokens so far.
I want to add the gas field in the RPC also for Tokens transfer, so that we avoid weird answers from the node. I'll do this once you have a first look at this code.

packages/fether-react/src/utils/estimateGas.js Outdated Show resolved Hide resolved
packages/fether-react/src/Send/TxForm/TxForm.js Outdated Show resolved Hide resolved
packages/fether-react/src/Send/TxForm/TxForm.js Outdated Show resolved Hide resolved
packages/fether-react/src/Send/TxForm/TxForm.js Outdated Show resolved Hide resolved
@amaury1093
Copy link
Collaborator

For future reference, if anyone is reading this PR:
An alternative, cleaner but more complicated solution would be to put gas as a derived form value, using https://github.com/final-form/final-form-calculate, for a future PR.

@Tbaut
Copy link
Collaborator Author

Tbaut commented Dec 10, 2018

Let me know what you think about that, I could use final-form-calculate, it works as expected for ETH and tokens, but I can't get rid of a warning message: Warning: Form decorators should not change from one render to the next as new values will be ignored

Copy link
Collaborator

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

It's cleaner imo, but estimateGas() shouldn't be called in validateAmount() anymore.

packages/fether-react/src/Send/TxForm/TxForm.js Outdated Show resolved Hide resolved
packages/fether-react/src/Send/TxForm/TxForm.js Outdated Show resolved Hide resolved
packages/fether-react/src/Send/TxForm/TxForm.js Outdated Show resolved Hide resolved
packages/fether-react/src/Send/TxForm/TxForm.js Outdated Show resolved Hide resolved
packages/fether-react/src/Send/TxForm/TxForm.js Outdated Show resolved Hide resolved
@Tbaut Tbaut changed the title [WIP] Support exact amount for account to account transactions Support exact amount for account to account transactions Dec 11, 2018
@Tbaut
Copy link
Collaborator Author

Tbaut commented Dec 11, 2018

Thanks a lot, I addressed the comments and it should look better now. No more error, token transfer also have gas in postTransaction now.

@Tbaut Tbaut changed the title Support exact amount for account to account transactions Add gas estimation to the postTransaction RPC Dec 11, 2018
Copy link
Collaborator

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

nice! Only some grumbles left

packages/fether-react/src/Send/TxForm/TxForm.js Outdated Show resolved Hide resolved
packages/fether-react/src/Send/TxForm/TxForm.js Outdated Show resolved Hide resolved
packages/fether-react/src/Send/TxForm/TxForm.js Outdated Show resolved Hide resolved
packages/fether-react/src/Send/TxForm/TxForm.js Outdated Show resolved Hide resolved
@Tbaut
Copy link
Collaborator Author

Tbaut commented Dec 11, 2018

Thanks again, comments addressed :)

Copy link
Collaborator

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

Sorry, found 2 more small grumbles

packages/fether-react/src/Send/TxForm/TxForm.js Outdated Show resolved Hide resolved
packages/fether-react/src/Send/TxForm/TxForm.js Outdated Show resolved Hide resolved
@Tbaut
Copy link
Collaborator Author

Tbaut commented Dec 11, 2018

No problem, I think it makes a lot of sense and simplifies things quite a bit. I've tested, as always, and it looks like it works :)

Copy link
Collaborator

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

🎉

@amaury1093 amaury1093 merged commit b0d5a65 into master Dec 11, 2018
@amaury1093 amaury1093 deleted the tbaut-estimate-enhancement branch December 11, 2018 13:51
@axelchalon
Copy link
Contributor

👍

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

Successfully merging this pull request may close these issues.

Get to the password screen with not enough funds
3 participants