Separate fee handling for inner tx of type move balance #5627

sstanculeanu · 2023-10-04T13:45:16Z

Reasoning behind the pull request

relayed move balance uses fee for processing

Proposed changes

use real move balance fee

Testing procedure

with feat branch

Pre-requisites

Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:

was the PR targeted to the correct branch?
if this is a larger feature that probably needs more than one PR, is there a feat branch created?
if this is a feat branch merging, do all satellite projects have a proper tag inside go.mod?

codecov · 2023-10-17T06:45:10Z

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (fe4c31c) 80.05% compared to head (c64461a) 80.05%.
Report is 1 commits behind head on feat/relayedv3.

❗ Current head c64461a differs from pull request most recent head 21fd96a. Consider uploading reports for the commit 21fd96a to get more accurate results

Additional details and impacted files

@@                Coverage Diff                 @@
##           feat/relayedv3    #5627      +/-   ##
==================================================
- Coverage           80.05%   80.05%   -0.01%     
==================================================
  Files                 706      706              
  Lines               93789    93809      +20     
==================================================
+ Hits                75081    75096      +15     
- Misses              13369    13372       +3     
- Partials             5339     5341       +2

Files	Coverage Δ
common/enablers/enableEpochsHandler.go	`100.00% <100.00%> (ø)`
common/enablers/epochFlags.go	`98.73% <100.00%> (+0.01%)`	⬆️
genesis/process/shardGenesisBlockCreator.go	`77.09% <100.00%> (+0.03%)`	⬆️
node/metrics/metrics.go	`100.00% <100.00%> (ø)`
process/transaction/baseProcess.go	`83.66% <100.00%> (+0.32%)`	⬆️
process/transaction/shardProcess.go	`73.90% <76.92%> (-0.03%)`	⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sasurobert · 2023-10-24T08:30:08Z

integrationTests/multiShard/relayedTx/common.go

@@ -286,3 +290,15 @@ func GetUserAccount(
 	}
 	return nil
 }
+func subFeesFromRelayer(tx, userTx *transaction.Transaction, economicsFee process.FeeHandler, relayer *integrationTests.TestWalletAccount) {


add empty line before

sasurobert · 2023-10-24T10:09:10Z

process/transaction/shardProcess.go

+		shouldConsiderMoveBalanceFee := dstShardTxType == process.MoveBalance &&
+			txProc.enableEpochsHandler.IsFixRelayedMoveBalanceFlagEnabled()
+		if shouldConsiderMoveBalanceFee {
+			totalCost = txProc.economicsFee.ComputeMoveBalanceFee(tx)


this is not correct. Rewriting the totalCost to moveBalance - will only add the move balance part, and it will not use the gasLimit of the transaction. So it is only 50K + 1.5K*len(txData). You need to use ComputeTxFee(tx data.TransactionWithFeeHandler) *big.Int function. This handles correctly. Do this on all this implementation

pushed, but as seen after tests it does not reach the desired behavior.. looking for another solution

AdoAdoAdo · 2023-10-05T13:54:31Z

integrationTests/vm/txsFee/guardAccount_test.go

@@ -962,15 +962,15 @@ func TestGuardAccounts_RelayedTransactionV1(t *testing.T) {
 		alice,
 		david,
 		gasPrice,
-		transferGas+guardianSigVerificationGas,
+		1+guardianSigVerificationGas,


what is that magic 1?
maybe have a variable instead?

AdoAdoAdo · 2023-10-25T07:12:38Z

integrationTests/multiShard/relayedTx/edgecases/edgecases_test.go

@@ -151,3 +149,16 @@ func checkPlayerBalancesWithPenalization(
 		assert.Equal(t, userAcc.GetNonce(), players[i].Nonce)
 	}
 }
+
+func appendFeeToTotalFees(relayerTx, userTx *transaction.Transaction, economicsData process.EconomicsDataHandler, totalFees *big.Int) {
+	if len(userTx.Data) == 0 { // move balance


can you remove the else and do a return on the if instead?

integrationTests/vm/txsFee/relayedMoveBalance_test.go

AdoAdoAdo · 2023-10-25T07:26:47Z

process/transaction/baseProcess.go

@@ -145,7 +146,13 @@ func (txProc *baseTxProcessor) checkTxValues(
 		if tx.GasLimit < txProc.economicsFee.ComputeGasLimit(tx) {


can you extract what is inside if isUserTxOfRelayed into a method computeRelayedUserTxFee
you can then use this in the shard process as well.

process/transaction/baseProcess.go

AdoAdoAdo · 2023-11-01T13:14:58Z

integrationTests/vm/txsFee/relayedBuiltInFunctions_test.go

-	require.Equal(t, vmcommon.Ok, retCode)
-	require.Nil(t, err)
+		rtxData := integrationTests.PrepareRelayedTxDataV1(innerTx)
+		rTxGasLimit := 1 + gasLimit + uint64(len(rtxData))


there are still some magic 1's here and in other places.
Could you replace with some constant/variable?

done, replaced everywhere

AdoAdoAdo · 2023-11-01T13:15:28Z

integrationTests/vm/txsFee/relayedESDT_test.go

-	rTxGasLimit := 1 + gasLimit + uint64(len(rtxData))
-	rtx := vm.CreateTransaction(0, innerTx.Value, relayerAddr, sndAddr, gasPrice, rTxGasLimit, rtxData)
+		rtxData := integrationTests.PrepareRelayedTxDataV1(innerTx)
+		rTxGasLimit := 1 + gasLimit + uint64(len(rtxData))


here as well there is a magic 1

AdoAdoAdo · 2023-11-01T13:15:48Z

integrationTests/vm/txsFee/guardAccount_test.go

@@ -1124,13 +1125,13 @@ func TestGuardAccounts_RelayedTransactionV2(t *testing.T) {
 		alice,
 		david,
 		gasPrice,
-		transferGas+guardianSigVerificationGas,
+		1,


can you replace with some variable?

AdoAdoAdo · 2023-11-01T13:16:28Z

integrationTests/vm/txsFee/relayedScCalls_test.go

-	require.Equal(t, vmcommon.Ok, retCode)
-	require.Nil(t, err)
+		rtxData := integrationTests.PrepareRelayedTxDataV1(userTx)
+		rTxGasLimit := 1 + gasLimit + uint64(len(rtxData))


can you replace the 1 with a variable instead?

also valid in other places in this file

added separate fee handling for inner tx of type move balance

34f868d

sstanculeanu self-assigned this Oct 4, 2023

Base automatically changed from relayedV3 to feat/relayedv3 October 11, 2023 07:05

Merge branch 'feat/relayedv3' into relayed_move_balance

62e4adc

sstanculeanu marked this pull request as ready for review October 17, 2023 13:17

sstanculeanu added type:bug Something isn't working area:processing impact:low interested:protocol complexity:medium labels Oct 17, 2023

sasurobert requested changes Oct 24, 2023

View reviewed changes

sstanculeanu added 2 commits October 26, 2023 18:08

fixes after review

a3014e6

fix tests

ff9f169

AdoAdoAdo reviewed Oct 27, 2023

View reviewed changes

sstanculeanu added 2 commits October 31, 2023 11:28

further fixes after review + proper fee fix

828e69d

fixed tests when running with race

9f76f80

AdoAdoAdo reviewed Nov 1, 2023

View reviewed changes

fixes after review

21fd96a

sasurobert approved these changes Nov 16, 2023

View reviewed changes

AdoAdoAdo approved these changes Nov 27, 2023

View reviewed changes

sstanculeanu merged commit 564dc15 into feat/relayedv3 Nov 27, 2023
5 checks passed

sstanculeanu deleted the relayed_move_balance branch November 27, 2023 12:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate fee handling for inner tx of type move balance #5627

Separate fee handling for inner tx of type move balance #5627

sstanculeanu commented Oct 4, 2023 •

edited

Loading

codecov bot commented Oct 17, 2023 •

edited

Loading

sasurobert Oct 24, 2023

sasurobert Oct 24, 2023

sstanculeanu Oct 26, 2023 •

edited

Loading

AdoAdoAdo Oct 5, 2023

AdoAdoAdo Oct 25, 2023

AdoAdoAdo Oct 25, 2023

AdoAdoAdo Nov 1, 2023

sstanculeanu Nov 13, 2023

AdoAdoAdo Nov 1, 2023

AdoAdoAdo Nov 1, 2023

AdoAdoAdo Nov 1, 2023

		@@ -145,7 +146,13 @@ func (txProc *baseTxProcessor) checkTxValues(
		if tx.GasLimit < txProc.economicsFee.ComputeGasLimit(tx) {

Separate fee handling for inner tx of type move balance #5627

Separate fee handling for inner tx of type move balance #5627

Conversation

sstanculeanu commented Oct 4, 2023 • edited Loading

Reasoning behind the pull request

Proposed changes

Testing procedure

Pre-requisites

codecov bot commented Oct 17, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sstanculeanu Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sstanculeanu commented Oct 4, 2023 •

edited

Loading

codecov bot commented Oct 17, 2023 •

edited

Loading

sstanculeanu Oct 26, 2023 •

edited

Loading