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 gas estimation error if lacking cUSD to pay for fees #1292

Closed
wants to merge 1 commit into from

Conversation

oneeman
Copy link
Contributor

@oneeman oneeman commented Jan 11, 2021

In order to be able to pay the fees for a transaction, you must have at least the amount of the fee, you shouldn't need to have strictly more than that amount.

This was leading to spurious errors during gas estimation. For gas estimation, the gas price is set to zero, and so canPayFee() should always return true, even if the account has no funds. However, it was returning false if using cUSD as the fee currency. If using Celo as the fee currency, it was returning true, but that is due to the upstream hack which sets the account's balance to maxuint (and which has been changed in upstream v1.9.13 and we will merge in PR #1251).

Description

If an account doesn't have enough funds to pay transaction fees, it should fail when sending the transaction, not during gas estimation. This PR fixes a logic bug that was causing it to fail during gas estimation when the fee currency was cUSD and the account had no cUSD. Note, too, that after the changes in #1251 (not merged to master as of this moment), which removes the upstream maxuint hack, the bug would also have appeared if the fee currency had been CELO.

Tested

Here are the results before and after this fix. This was done by attempting to make a transaction using celo-cli, with the fee currency set to cUSD and an account that has no cUSD.

Before the fix:

Sending Transaction: voteTx... !
    Error: Gas estimation failed: Could not decode transaction failure reason

After the fix:

Sending Transaction: voteTx... !
    Error: insufficient funds for gas * price + value + gatewayFee

In order to be able to pay the fees for a transaction, you have to have at least the amount of the fee, not strictly more than that amount.

This was leading to spurious errors during gas estimation.  For gas estimation, the gas price is set to zero, and so `canPayFee()` should always return true, even if the account has no funds.  However, it was returning false if using CUSD as the fee currency.  If using CELO as the fee currency, it was returning true, but that is due to the upstream hack which sets the account's balance to maxuint (and which has been changed in upstream v1.9.13 and we will merge in PR #1251).
@oneeman oneeman requested a review from kevjue as a code owner January 11, 2021 16:52
@oneeman oneeman changed the title Fix gas estimation error due lacking cUSD to pay for fees Fix gas estimation error if lacking cUSD to pay for fees Jan 11, 2021
@oneeman
Copy link
Contributor Author

oneeman commented Jan 11, 2021

As a side note, the error message insufficient funds for gas * price + value + gatewayFee isn't ideal, because when the fee currency isn't Celo, you're not actually adding together the fee and the transaction value. In the 1.9.14 merge there are some small changes to these errors, so I will check there how to make them clearer.

@oneeman
Copy link
Contributor Author

oneeman commented Jan 11, 2021

@nategraf suggested this may require a hard fork, and looking at it again, it seems like it would. Going to convert to a draft for now.

@oneeman
Copy link
Contributor Author

oneeman commented Jan 21, 2021

Closing in favor of #1310

@oneeman oneeman closed this Jan 21, 2021
@oneeman oneeman deleted the oneeman/fix-canPayFee-check branch February 17, 2021 19:38
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.

1 participant