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

Add gas estimation to the postTransaction RPC - #279

Merged
amaury1093 merged 15 commits into
masterfrom
tbaut-estimate-enhancement
Dec 11, 2018
Merged

Add gas estimation to the postTransaction RPC#279
amaury1093 merged 15 commits into
masterfrom
tbaut-estimate-enhancement

Conversation

@Tbaut

@Tbaut Tbaut commented Dec 6, 2018

Copy link
Copy Markdown
Collaborator

@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

Tbaut commented Dec 7, 2018

Copy link
Copy Markdown
Collaborator Author

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.

Comment thread packages/fether-react/src/utils/estimateGas.js Outdated
Comment thread packages/fether-react/src/Send/TxForm/TxForm.js Outdated
Comment thread packages/fether-react/src/Send/TxForm/TxForm.js Outdated
Comment thread packages/fether-react/src/Send/TxForm/TxForm.js Outdated
@amaury1093

Copy link
Copy Markdown
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

Tbaut commented Dec 10, 2018

Copy link
Copy Markdown
Collaborator Author

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

@amaury1093 amaury1093 left a comment

Copy link
Copy Markdown
Collaborator

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.

Comment thread packages/fether-react/src/Send/TxForm/TxForm.js Outdated
Comment thread packages/fether-react/src/Send/TxForm/TxForm.js Outdated
Comment thread packages/fether-react/src/Send/TxForm/TxForm.js Outdated
Comment thread packages/fether-react/src/Send/TxForm/TxForm.js Outdated
Comment thread packages/fether-react/src/Send/TxForm/TxForm.js Outdated
@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

Tbaut commented Dec 11, 2018

Copy link
Copy Markdown
Collaborator Author

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

@amaury1093 amaury1093 left a comment

Copy link
Copy Markdown
Collaborator

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

Comment thread packages/fether-react/src/Send/TxForm/TxForm.js Outdated
Comment thread packages/fether-react/src/Send/TxForm/TxForm.js Outdated
Comment thread packages/fether-react/src/Send/TxForm/TxForm.js Outdated
Comment thread packages/fether-react/src/Send/TxForm/TxForm.js Outdated
Comment thread packages/fether-react/src/Send/TxForm/TxForm.js Outdated
@Tbaut

Tbaut commented Dec 11, 2018

Copy link
Copy Markdown
Collaborator Author

Thanks again, comments addressed :)

@amaury1093 amaury1093 left a comment

Copy link
Copy Markdown
Collaborator

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

Comment thread packages/fether-react/src/Send/TxForm/TxForm.js Outdated
Comment thread packages/fether-react/src/Send/TxForm/TxForm.js Outdated
@Tbaut

Tbaut commented Dec 11, 2018

Copy link
Copy Markdown
Collaborator Author

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 :)

@amaury1093 amaury1093 left a comment

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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