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

Handle intra-shard relayed transactions with signal error #81

Merged
merged 13 commits into from
Sep 4, 2023

Conversation

andreibancioiu
Copy link
Contributor

@andreibancioiu andreibancioiu commented Aug 29, 2023

While the balance changing operations are perfectly recoverable from most kinds of relayed transactions (given the smart contract results generated by the protocol), there is a case where this is only possible by also unwrapping and inspecting the inner transaction: when the relayed transaction is completely intra-shard (both the wrapping transaction and the inner transaction are intra-shard, in the same shard), and the processing of the transaction results into a contract signal error.

The PR handles this exotic case and emits an extra balance-changing operation.

@andreibancioiu andreibancioiu self-assigned this Aug 29, 2023
@codecov
Copy link

codecov bot commented Aug 29, 2023

Codecov Report

Patch coverage: 84.41% and project coverage change: +0.54% 🎉

Comparison is base (9417edc) 71.06% compared to head (0b8a095) 71.60%.

❗ Current head 0b8a095 differs from pull request most recent head f524b19. Consider uploading reports for the commit f524b19 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #81      +/-   ##
==========================================
+ Coverage   71.06%   71.60%   +0.54%     
==========================================
  Files          32       33       +1     
  Lines        1683     1747      +64     
==========================================
+ Hits         1196     1251      +55     
- Misses        407      411       +4     
- Partials       80       85       +5     
Files Changed Coverage Δ
server/services/errors.go 99.10% <ø> (ø)
server/provider/networkProvider.go 45.53% <75.00%> (+1.20%) ⬆️
server/services/relayedTransactions.go 75.00% <75.00%> (ø)
server/services/transactionsTransformer.go 63.34% <80.00%> (+1.80%) ⬆️
server/services/transactionEventsController.go 56.92% <87.50%> (+4.29%) ⬆️
server/services/common.go 73.52% <100.00%> (+3.52%) ⬆️
server/services/transactionFilters.go 76.19% <100.00%> (-1.09%) ⬇️
server/services/transactionsFeaturesDetector.go 94.11% <100.00%> (+2.81%) ⬆️

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

@AdoAdoAdo AdoAdoAdo self-requested a review August 29, 2023 09:59
server/factory/components/observerFacade.go Show resolved Hide resolved
@@ -50,6 +50,14 @@ func isZeroAmount(amount string) bool {
return false
}

func isZeroBigInt(value *big.Int) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you rename?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed to isZeroBigIntOrNil.

server/services/transactionsTransformer.go Show resolved Hide resolved
return nil, err
}

if isZeroBigInt(&innerTx.Value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

for token transfers (the other feature branch) should we care also about non native token transfers?
if yes, maybe add a TODO?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. Ideally, the existing logic (in feat/esdt) that handles logs & events for ESDT transfers should be sufficient. Added an issue here, to check & confirm: #82.

AdoAdoAdo
AdoAdoAdo previously approved these changes Aug 30, 2023
AdoAdoAdo
AdoAdoAdo previously approved these changes Sep 4, 2023
miiu96
miiu96 previously approved these changes Sep 4, 2023
}

for _, event := range tx.Logs.Events {
isSignalError := event.Identifier == transactionEventSignalError
Copy link
Contributor

Choose a reason for hiding this comment

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

you can use this const from core core.SignalErrorOperation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed.

@andreibancioiu andreibancioiu dismissed stale reviews from miiu96 and AdoAdoAdo via f524b19 September 4, 2023 11:21
@andreibancioiu andreibancioiu merged commit 0ab9266 into main Sep 4, 2023
5 checks passed
@andreibancioiu andreibancioiu deleted the relayed-25 branch September 4, 2023 11:31
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.

3 participants