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
asyncCallback-with-value #2476
asyncCallback-with-value #2476
Conversation
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.
Mostly comments and questions.
@@ -1315,12 +1315,25 @@ func createBaseSCR( | |||
return result | |||
} | |||
|
|||
func addVMOutputResultsToSCR(vmOutput *vmcommon.VMOutput, result *smartContractResult.SmartContractResult) { |
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.
Suggest renaming to addCallbackInfoToSCR()
.
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.
This is much more clar what it does
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.
Is it always an asynchronousCallback callType?
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.
this function is called only for asynchronous callback
process/smartContract/process.go
Outdated
@@ -1332,6 +1345,12 @@ func (sc *scProcessor) createSmartContractResults( | |||
return []data.TransactionHandler{result} | |||
} | |||
|
|||
if callType == vmcommon.AsynchronousCall && bytes.Equal(outAcc.Address, tx.GetSndAddr()) { |
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.
Not sure, but this if
should come before if !sc.flagDeploy.IsSet()
, otherwise addVMOutputResultsToSCR()
will never be called for callbacks until we enable deployment (and set the flagDeploy
to true). The Delegation SC might fail.
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. But that will make kill backward compatibility. These changes can work properly if that are no asyncs calls from delegation to metachain
process/smartContract/process.go
Outdated
@@ -1358,6 +1378,12 @@ func (sc *scProcessor) createSmartContractResults( | |||
result.OriginalSender = tx.GetSndAddr() | |||
} | |||
|
|||
isAsyncTransferBackToSender := callType == vmcommon.AsynchronousCall && bytes.Equal(outAcc.Address, tx.GetSndAddr()) | |||
isLastOutTransfer := i == lenOutTransfers-1 | |||
if isLastOutTransfer && isAsyncTransferBackToSender { |
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.
Do we have the certainty that the async transfer back to sender is always the last one in a list of output transfers? This seems like a condition that can be broken accidentally, with later code changes.
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 is our standard. That is how we say this will work.
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.
If we define that the last transfer is sent back in the asyncCallback than everyone codes like that.
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.
Can this be enforced?
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.
if that is our defined rule in the code -> it means it is enforced.
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 will work as it is, and it's safe enough - tests will fail if we accidentally break this rule.
But if there is a stricter criterion possible (e.g. multiple conditions on the fields of outAcc
), we should use that.
@@ -1315,12 +1315,25 @@ func createBaseSCR( | |||
return result | |||
} | |||
|
|||
func addVMOutputResultsToSCR(vmOutput *vmcommon.VMOutput, result *smartContractResult.SmartContractResult) { |
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.
Is it always an asynchronousCallback callType?
txHash []byte, | ||
) *smartContractResult.SmartContractResult { | ||
scr := &smartContractResult.SmartContractResult{ | ||
Value: big.NewInt(0), |
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.
Should there be no way to send some value through these smart contract results?
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.
that is done when the asynccallback is done from an output transfer. If there is no output transfer than no value can be sent back.
process/smartContract/process.go
Outdated
@@ -1358,6 +1378,12 @@ func (sc *scProcessor) createSmartContractResults( | |||
result.OriginalSender = tx.GetSndAddr() | |||
} | |||
|
|||
isAsyncTransferBackToSender := callType == vmcommon.AsynchronousCall && bytes.Equal(outAcc.Address, tx.GetSndAddr()) | |||
isLastOutTransfer := i == lenOutTransfers-1 | |||
if isLastOutTransfer && isAsyncTransferBackToSender { |
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.
Can this be enforced?
) *smartContractResult.SmartContractResult { | ||
scr := &smartContractResult.SmartContractResult{ | ||
Value: big.NewInt(0), | ||
RcvAddr: tx.GetSndAddr(), |
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.
The scr receiver is actually the original sender right?
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.
the sender of the transaction. as this was an asyncCall - the sender and receiver switch places.
asynchronous callback with last output transfer