Proper fix for gasUsed and fee in case of relayedV3 #6440
Conversation
sstanculeanu
added
type:bug
|
📊 MultiversX Automated Test Report: View Report 🔄 Build Details:
🚀 Environment Variables:
|
| tx.InitiallyPaidFee = totalFee.String() | ||
| tx.GasUsed = big.NewInt(0).Div(totalFee, big.NewInt(0).SetUint64(tx.GasPrice)).Uint64() | ||
| } | ||
| initialTotalFee, isRelayed := gfp.getFeeOfRelayed(tx) |
There was a problem hiding this comment.
This is basically the same as before right?
There was a problem hiding this comment.
exactly, only removed one if
| @@ -69,7 +69,17 @@ func (gfp *gasUsedAndFeeProcessor) computeAndAttachGasUsedAndFee(tx *transaction | |||
|
|
|||
| gfp.setGasUsedAndFeeBaseOnRefundValue(tx, scr.Value) | |||
| hasRefundForSender = true | |||
| break | |||
| totalRefunds.Add(totalRefunds, scr.Value) | |||
There was a problem hiding this comment.
doesn't this affect regular transactions with multiple refund scrs from other shards as well?
There was a problem hiding this comment.
I see.. it means this was a bug also for that transactions.. updated to iterate through all of them
| } | ||
|
|
||
| hasRefundForSender := false | ||
| totalRefunds := big.NewInt(0) | ||
| isRelayedV3 := len(tx.InnerTransactions) > 0 | ||
| for _, scr := range tx.SmartContractResults { | ||
| if !scr.IsRefund || scr.RcvAddr != tx.Sender { |
There was a problem hiding this comment.
I think line 67 is dead code right?
there was already the same check done on line 63 with a continue.
There was a problem hiding this comment.
true, removed it
| @@ -167,12 +169,21 @@ func (tep *transactionsFeeProcessor) prepareTxWithResults(txHashHex string, txWi | |||
| txWithResults.GetFeeInfo().SetGasUsed(gasUsed) | |||
| txWithResults.GetFeeInfo().SetFee(fee) | |||
| hasRefund = true | |||
| break | |||
| totalRefunds.Add(totalRefunds, scr.Value) | |||
| if !isRelayedV3 { | |||
There was a problem hiding this comment.
this will affect other transactions with multiple scrs in different shards as well, so I think it should iterate over all.
There was a problem hiding this comment.
I see.. it means this was a bug also for that transactions.. updated to iterate through all of them
|
📊 MultiversX Automated Test Report: View Report 🔄 Build Details:
🚀 Environment Variables:
|
|
📊 MultiversX Automated Test Report: View Report 🔄 Build Details:
🚀 Environment Variables:
|
|
📊 MultiversX Automated Test Report: View Report 🔄 Build Details:
🚀 Environment Variables:
|
| @@ -682,6 +689,20 @@ func (ed *economicsData) ComputeGasLimitBasedOnBalanceInEpoch(tx data.Transactio | |||
| return totalGasLimit, nil | |||
| } | |||
|
|
|||
| func (ed *economicsData) computeRelayedTxV3MinGasLimit(tx data.TransactionWithFeeHandler) uint64 { | |||
There was a problem hiding this comment.
Non-trivial function, should also be unit-tested (separately from "ComputeGasUsedAndFeeBasedOnRefundValueInEpoch").
There was a problem hiding this comment.
right, will add tests in a separate PR
| func (ed *economicsData) computeRelayedTxV3MinGasLimit(tx data.TransactionWithFeeHandler) uint64 { | ||
| relayedTxGasLimit := ed.ComputeGasLimit(tx) | ||
| relayedTxMinGasLimit := ed.MinGasLimit() | ||
| relayedTxGasLimitDiff := relayedTxGasLimit - relayedTxMinGasLimit // this may be positive if the relayed tx is guarded |
There was a problem hiding this comment.
Is the case mentioned in comment tested using "TestEconomicsData_ComputeGasUsedAndFeeBasedOnRefundValueRelayedV3"?
There was a problem hiding this comment.
indeed not tested here, covered by testing scenarios 100%
will add tests in a separate PR
|
📊 MultiversX Automated Test Report: View Report 🔄 Build Details:
🚀 Environment Variables:
|
|
📊 MultiversX Automated Test Report: View Report 🔄 Build Details:
🚀 Environment Variables:
|
Reasoning behind the pull request
Proposed changes
Testing procedure
Pre-requisites
Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:
featbranch created?featbranch merging, do all satellite projects have a proper tag insidego.mod?