remove tx fee

[Ashuaidehao]

No description provided.

[shargon]

What's the problem?

[Ashuaidehao]

We use the rpc method calculate tx fee mostly, so the fake tx fee is not necessay.
If contract has some code to check tx fee then do something, it will block by the huge fake tx fee here.

[Jim8y]

We use the rpc method calculate tx fee mostly, so the fake tx fee is not necessay. If contract has some code to check tx fee then do something, it will block by the huge fake tx fee here.

My concern here is that removing a field from the interface might cause some chaos to other services who rely on it. Maybe we should mark it as deprecated for a while, instead of removing it directly.

[superboyiii]

My concern here is that removing a field from the interface might cause some chaos to other services who rely on it. Maybe we should mark it as deprecated for a while, instead of removing it directly.

As I know some dapps relying on it has been blocked already by this sysfee as it's 0 before, now it's using MaxGasInvoke...

[Jim8y]

If you say so.

[shargon]

We use the rpc method calculate tx fee mostly, so the fake tx fee is not necessay. If contract has some code to check tx fee then do something, it will block by the huge fake tx fee here.

Sorry but still don't understand the problem, the invocation is with this limited gas, so what's the problem if someone extract the tx and find the same gas?

[Ashuaidehao]

We use the rpc method calculate tx fee mostly, so the fake tx fee is not necessay. If contract has some code to check tx fee then do something, it will block by the huge fake tx fee here.

Sorry but still don't understand the problem, the invocation is with this limited gas, so what's the problem if someone extract the tx and find the same gas?

Just like this .

            var totalGas = tx.SystemFee + tx.NetworkFee;
            var estimateGases = (BigInteger[])Contract.Call(RouterContract, "getAmountsOut", CallFlags.All, amountIn, tokenPath);
            var estimateGas = estimateGases[^1];
            Assert(estimateGas > totalGas, "EstimateGas is not enough", estimateGas, totalGas);

[shargon]

We use the rpc method calculate tx fee mostly, so the fake tx fee is not necessay. If contract has some code to check tx fee then do something, it will block by the huge fake tx fee here.

Sorry but still don't understand the problem, the invocation is with this limited gas, so what's the problem if someone extract the tx and find the same gas?

Just like this .

            var totalGas = tx.SystemFee + tx.NetworkFee;
            var estimateGases = (BigInteger[])Contract.Call(RouterContract, "getAmountsOut", CallFlags.All, amountIn, tokenPath);
            var estimateGas = estimateGases[^1];
            Assert(estimateGas > totalGas, "EstimateGas is not enough", estimateGas, totalGas);

I see, but this case it's too specific, another project can check the SystemFee and if is it 0, return. I think that it's better to add something to allow to detect if it's a test or a real invocation.

[steven1227]

Any progress of this pull request? I think some contract invocation are blocked because of this change

[shargon]

I think that it's a very specific case. We should think in another solution, for example, configure the dummy transaction in a json

[erikzhang]

Assert(estimateGas > totalGas, "EstimateGas is not enough", estimateGas, totalGas);

And in a real transaction, the fee has been paid. Even if an exception is thrown, the fee cannot be refunded. So it doesn't make sense to make such an assert here.

[Ashuaidehao]

Assert(estimateGas > totalGas, "EstimateGas is not enough", estimateGas, totalGas);

And in a real transaction, the fee has been paid. Even if an exception is thrown, the fee cannot be refunded. So it doesn't make sense to make such an assert here.

Even after removed the assert, we still block by the default 20 gas here

            var receiverAmount = amountOut - totalGas - charges;
            if (receiverAmount > 0)
            {
                SafeTransfer(GAS.Hash, me, sender, receiverAmount);
            }
            else
            {
                onNotEnough(sender, amountOut, totalGas);
            }

Because the 20 gas is too big, most transactions will run into "else" in pre-execution condition and get a wrong estimate system fee, when the real tx is persist, they will go into "if" and fault.

[erikzhang]

Because the 20 gas is too big, most transactions will run into "else" in pre-execution condition and get a wrong estimate system fee, when the real tx is persist, they will go into "if" and fault.

Then I prefer Shargon‘s solution.

I think that it's better to add something to allow to detect if it's a test or a real invocation.

[steven1227]

Any detailed solution? Actually, my dapp is currently stuck because of this..

[erikzhang]

How about allowing the contract to control the process during pre-execution?

I haven't thought of a good way yet.

[roman-khimov]

1. In NeoGo test invocations use transactions with both fees set to zero, just like it was before aa730fb.

2. I still don't understand why contract cares about fees. I can't see all of it, but if we're talking about Flamingo, it probably cares about amountIn/amountOut balance, while fees are just out of its scope.

3. add something to allow to detect if it's a test or a real invocation.

   Any test/real branches are better be avoided, it can lead to wrong system fee estimates and wrong execution result estimates.

[erikzhang]

Any test/real branches are better be avoided, it can lead to wrong system fee estimates and wrong execution result estimates.

I agree. Then we can't set fee to 0 in test calls. Otherwise, contracts can detect if it's a test call by checking if the fee is 0.

[erikzhang]

Then we can't set fee to 0 in test calls. Otherwise, contracts can detect if it's a test call by checking if the fee is 0.

One solution I can think of is two test executions per transaction. The first time the fee is set to 0, and the contract is allowed to control the branches. The second time the fee is set to the actual gas consumed, so that the contract cannot control the branches.

[roman-khimov]

How about the network fee? Any static value there will also be wrong.

What I'd do here is merge this PR as is to solve the problem some dApps have now and create a new issue for additional discussion. The behavior was changed, this change broke some (buggy) contracts, reverting it won't affect good contracts (they don't care about fees), but will restore the old behavior and make contracts that depend on it work. Then we can try to fix contracts/create a better solution for 3.4.1.

[erikzhang]

We can merge it first and consider #746 in the next release.