Deduct fees on failed transactions #1012
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1012 +/- ##
==========================================
- Coverage 55.58% 55.55% -0.03%
==========================================
Files 481 481
Lines 29434 29449 +15
==========================================
+ Hits 16360 16361 +1
- Misses 10843 10854 +11
- Partials 2231 2234 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
…fees-on-failed-transactions
…fees-on-failed-transactions
…fees-on-failed-transactions
…:onflow/flow-go into janez/deduct-fees-on-failed-transactions
| // applying contract changes | ||
| // this writes back the contract contents to accounts | ||
| // if any error occurs we fail the tx | ||
| // this needs to happen before checking limits, so that contract changes are committed to the state | ||
| updatedKeys, err := env.Commit() | ||
| if err != nil && txError == nil { | ||
| txError = fmt.Errorf("transaction invocation failed: %w", err) |
There was a problem hiding this comment.
So if we had feesError we ignore this newly assigned error here?
There was a problem hiding this comment.
That is correct. If there is a error with deducting fees (or with checking storage) the tx error should be hidden from the user. But we should probably still log it... I will add that.
| // reset env | ||
| env = NewTransactionEnvironment(*ctx, vm, sth, programs, proc.Transaction, proc.TxIndex, span) | ||
|
|
||
| // try to deduct fees again, to get the fee deduction events |
There was a problem hiding this comment.
We could re-organise this so no second fees deduction code is run.
Something along the lines of
actualDelta = delta.NewChild()
err = runTx(actualDelta)
if err == nil {
delta.Merge(actualDelta)
}
delta.Commit()
|
I might miss something, but we can't we just check the account balance before running a tx and simply fail it if the balance won't cover the fee? |
|
The transaction could move funds to/from the payer account, so there is no way to know for sure if the payer will have enough funds before actually executing the transaction. |
…fees-on-failed-transactions
…fees-on-failed-transactions
…fees-on-failed-transactions
…onflow/flow-go into janez/deduct-fees-on-failed-transactions
…rating-over-unordered-list
…rating-over-unordered-list
…onflow/flow-go into janez/fix-iterating-over-unordered-list
…onflow/flow-go into janez/deduct-fees-on-failed-transactions
janezpodhostnik
changed the base branch from
master
to
janez/fix-iterating-over-unordered-list
…fees-on-failed-transactions
…fees-on-failed-transactions
|
bors try |
tryBuild failed: |
…fees-on-failed-transactions
|
bors merge |
closes: https://github.com/dapperlabs/flow-internal/issues/1449
Deduct fees on failed transactions
If transaction fails still try to deduct fees. Consequences of this change:
There is currently an edge case: if the account minimum balance is less than the transaction fees the balance does not get reduced to 0. PR onflow/flow-core-contracts#221 addresses that issue.
DoD Checklist