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

unbreak paratime withdraw #228

Merged
merged 6 commits into from
Jan 21, 2022
Merged

unbreak paratime withdraw #228

merged 6 commits into from
Jan 21, 2022

Conversation

pro-wh
Copy link
Contributor

@pro-wh pro-wh commented Jan 20, 2022

proposed upcoming changes to Emerald will require a minimum gas price, which will make zero-fee withdrawal stop working

this is a minimal patch to use a hardcoded value for a withdrawal fee (that being the only non-deposit paratime transaction supported). we're interested in querying the minimum gas price in a larger change #227

reviewers, be on your guard. I don't know if I'm right about these changes

@pro-wh
Copy link
Contributor Author

pro-wh commented Jan 20, 2022

tried on testnet and the balances seem roughly right, but oasisscan isn't indexing the transactions

@pro-wh pro-wh force-pushed the pro-wh/feature/minprice10 branch from e0c9819 to 5b3e398 Compare January 20, 2022 20:25
@pro-wh
Copy link
Contributor Author

pro-wh commented Jan 20, 2022

reduced paratime withdraw/deposit gas to 15k (from 50k)

added changelog

Copy link
Contributor

@nhynes nhynes left a comment

Choose a reason for hiding this comment

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

This should do. One suggestion though: add a popup that says "submit this transaction anyway" instead of forcing people to accept the local sanity check. That local check introduces a huge dependency on Oasisscan (or other indexer) and prevents people from moving their funds.

src/background/api/txHelper.js Show resolved Hide resolved
src/background/api/txHelper.js Show resolved Hide resolved
src/popup/pages/Send/index.js Show resolved Hide resolved
src/popup/pages/Send/index.js Show resolved Hide resolved
@pro-wh
Copy link
Contributor Author

pro-wh commented Jan 21, 2022

filed #233 for the "submit anyway" idea, thanks for the suggestion

Copy link
Member

@tjanez tjanez left a comment

Choose a reason for hiding this comment

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

Great job on deciphering all those small gas/fee units and correcting the computations!

One nit. Would it be possible to remove the separate "add changelog" commit and add each Change Log bullet to the commit that fixes it?

@pro-wh
Copy link
Contributor Author

pro-wh commented Jan 21, 2022

ya sure

The fee entry UI specifically deals with nano (1e-9) units, not
base units. In paratimes that have decimals != 9, this produced
the wrong gas amount.
The fee amount is the total fee. We shouldn't multiply it by the
gas.
@pro-wh
Copy link
Contributor Author

pro-wh commented Jan 21, 2022

oh some of the later commits don't even have changelog entries. I'll add those too

An upcoming change to Emerald will require a minimum gas price.
An estimate from the SDK CLI indicated that it was ~11287 gas.
@pro-wh pro-wh force-pushed the pro-wh/feature/minprice10 branch from abde87e to 2691cf2 Compare January 21, 2022 20:31
@pro-wh pro-wh merged commit 542bb4b into master Jan 21, 2022
@pro-wh pro-wh deleted the pro-wh/feature/minprice10 branch January 21, 2022 20:53
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.

3 participants