-
Notifications
You must be signed in to change notification settings - Fork 198
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-createSCRIfError-for-all-cases #2272
Conversation
scr.GasPrice = tx.GetGasPrice() | ||
|
||
gasToLock := sc.asyncCallStepCost + sc.asyncCallbackGasLock | ||
if tx.GetGasLimit() >= gasToLock { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if gasToLock < tx.GetGasLimit() ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all the gasLimit is consumed here. in the cross shard it will have only the tx.Value added back to the original sender, and that's it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean gasToLock > tx.GetGasLimit()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be the situation that asyncCallback to not be executed because the tx.GetGasLimit() < gasToLock, and in this way the tx value will not be returned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case the asyncCallback reaches the initial sender shard and value is returned. Even in case of error an asyncCallBack will add the txValue to the sender.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally, tx.GetGasLimit() < gasToLock
cannot happen, because Arwen performs this check before initiating the asyncCall()
. It should still be verified here, though.
@@ -2218,3 +2221,7 @@ func TestScProcessor_penalizeUserIfNeededShouldWorkOnFlagActivation(t *testing.T | |||
sc.penalizeUserIfNeeded(&transaction.Transaction{}, []byte("txHash"), callType, gasProvided, vmOutput) | |||
assert.Equal(t, uint64(0), vmOutput.GasRemaining) | |||
} | |||
|
|||
func TestSCProcessor_createSCRWhenError(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty unit test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doing it right now :D
scr.GasPrice = tx.GetGasPrice() | ||
|
||
gasToLock := sc.asyncCallStepCost + sc.asyncCallbackGasLock | ||
if tx.GetGasLimit() >= gasToLock { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally, tx.GetGasLimit() < gasToLock
cannot happen, because Arwen performs this check before initiating the asyncCall()
. It should still be verified here, though.
createSCRIfError remained as an old code. When smart contract deployment is enabled, asynchronous callbacks cases must be handled correctly. Thus added it.