Skip to content
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

Merged
merged 5 commits into from Nov 17, 2020
Merged

Conversation

sasurobert
Copy link
Contributor

asynchronous callback with last output transfer

Copy link
Contributor

@camilbancioiu camilbancioiu left a 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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest renaming to addCallbackInfoToSCR().

Copy link
Contributor Author

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

Copy link
Contributor

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?

Copy link
Contributor Author

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

@@ -1332,6 +1345,12 @@ func (sc *scProcessor) createSmartContractResults(
return []data.TransactionHandler{result}
}

if callType == vmcommon.AsynchronousCall && bytes.Equal(outAcc.Address, tx.GetSndAddr()) {
Copy link
Contributor

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.

Copy link
Contributor Author

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 Show resolved Hide resolved
@@ -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 {
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be enforced?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

process/smartContract/process.go Outdated Show resolved Hide resolved
process/smartContract/process.go Outdated Show resolved Hide resolved
@raduchis raduchis self-requested a review November 16, 2020 21:58
process/smartContract/process.go Outdated Show resolved Hide resolved
process/smartContract/process.go Outdated Show resolved Hide resolved
process/smartContract/process.go Outdated Show resolved Hide resolved
@@ -1315,12 +1315,25 @@ func createBaseSCR(
return result
}

func addVMOutputResultsToSCR(vmOutput *vmcommon.VMOutput, result *smartContractResult.SmartContractResult) {
Copy link
Contributor

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),
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be enforced?

process/smartContract/process.go Show resolved Hide resolved
) *smartContractResult.SmartContractResult {
scr := &smartContractResult.SmartContractResult{
Value: big.NewInt(0),
RcvAddr: tx.GetSndAddr(),
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@sasurobert sasurobert merged commit 943969e into feat/esdt-relayed-arwen Nov 17, 2020
@sasurobert sasurobert deleted the asyncCallback-with-value branch November 17, 2020 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants