Added transaction processed status #386

iulianpascalau · 2023-05-22T12:34:29Z

added transaction processed status

bogdan-rosianu · 2023-05-22T13:06:06Z

data/transaction.go

+//   ProcessedStatus: will contain the transaction status after local processing
+type ExtendedApiTransactionResult struct {
+	*transaction.ApiTransactionResult
+	ProcessedStatus transaction.TxStatus `json:"processingStatus,omitempty"`


might add a TODO to move it into mx-chain-core-go for rc/v1.6.0

andreibancioiu · 2023-05-22T12:41:43Z

process/testdata/pendingNewSCCall.json

+  "receiver": "erd1qqqqqqqqqqqqqpgq50dge6rrpcra4tp9hl57jl0893a4r2r72jpsk39rjj",
+  "sender": "erd1nfqu46k8ffc2zkzme3dunh675sv69vnjp605c8pk77akk4f3e6kq5hegux",


Ideally, we should use dummy addresses - at least for user addresses.

done, even though, our blockchain is public by design

Indeed, but at least search engines won't match the Github page when searching for a real address.

andreibancioiu · 2023-05-22T12:42:58Z

process/testdata/finishedOKSCCall.json

+  "status": "success",
+  "tokens": [
+    "RIDE-7d18e9",
+    "SRIDE-4ab1d4-0695ed"


Ideally, we should use invented token IDs (can also stay like this, though).

andreibancioiu · 2023-05-22T12:46:53Z

process/testdata/executingSCCall.json

+    "gasPrice": 1000000000,
+    "gasLimit": 40000000,
+    "gasUsed": 26406638,
+    "data": "Y2xhaW1SZXdhcmRzQDM2QDEyNDU3NDY5YzYzMDEzYzBiNWJjMWJhNEAxMjQ1NzQ2OWM2MzAxM2MwYjViYzFiYTRANzYzNGU1ZjJjMjAzMWI1MDE4ZTk1MDFiMzg4M2VkNmViNThlZjRkZmIyMmYwYmVkNGNiYmY0NTdhYjRlN2RiYjk0NzAwYTVjMjQ3OTBiYzQ2ZjdiMGZiNjI1Yjg2MmUzMDhmYWI2ZTkwYjJmNzZmYzU0MDZlOTQzYTE1NjY1MDhAMzdAMTIyZDIwODQ2NmJjNTVmODYzYWM1MTc2QDEyMmQyMDg0NjZiYzU1Zjg2M2FjNTE3NkBmZTJhNWJkNzA3MmU0MDg1MDMyMmJlNTdiNmZkMDQ5ZjdmOTYzOTI0YTk5ZWVhNDA5MDI4MDUyZDczYTYwZjI4MTgzZmI4NTg3NzgwZWU4OTAxMmRjMTM5YzEwYzVkZGYwYWNlYzhlODRkZGMyNThjYjNkMDIzYTE4ODQ5MTAwMkAzOEAxMjRlMGMxM2Q1MjRjNDRjNjQxMjY5NmZAMTI0ZTBjMTNkNTI0YzQ0YzY0MTI2OTZmQDM5N2ZhOGJmZjRiZGI5Y2RhODBhOGM4MDA3MzgxNzAwMGRlOTdjYTE4ZDQ2YWI0N2U0ZDYwNzg1MWIyYzgwYmExOGM3MWNkNzA5MWYwOGM4NjJlNjBiM2ZiOTA2MDViNzFhOWMzNWJlZDkxYzM4ZDQwZWRmMjA2MjIzNGUzNzAzQDM5QDEzOGMxMmVhYzg0ZTYwMTUwNGJmZjhiNEAxMzhjMTJlYWM4NGU2MDE1MDRiZmY4YjRANTlkODM3MDI5MzBiNTdkZjViNGI5YmQ4M2U1NjA5ZjJjYTFmY2VlZDdiMDY0NmY5MjEzMjEwNDVjNjA0Y2Y1YTY5YzM3MjYyNzVjNThlMjI5OGIzMDBlNjg2ZDVkNjBjZDFjOTE1YmRkNzkwMGE2ODJlNTJmYzk1NzUyMDg0MDFAM2FAMTM3MWMzODQ4N2U2MmZlMGY5YTVjNDc0QDEzNzFjMzg0ODdlNjJmZTBmOWE1YzQ3NEBhZjRiOTI1ZDZhODZlNGE3ZDFjNGMzOWM1ZGJjNjUzY2MyOTAxMjUyNzg3YjMxNmM3MDMwY2NkZjU5ODM3N2E4Y2NlNTEyOTdkMTAxYTRhMDM1ZWFmZWUzYWM1NDU0ODdiMzE5ZTlmNDg2YzdjZmJkZjM3MTU5Mjk4NmQ0YzkwOQ==",


Fields that aren't involved into the computation of status can be omitted. However, if these test files will be used by some other tests, they can remain, of course (opinion).

curated the test file

andreibancioiu · 2023-05-22T13:29:12Z

process/transactionProcessor.go

+	txCompleted            = "completedTxEvent"
+	txFailed               = "signalError"
+	scDeploy               = "SCDeploy"


The constants can be prefixed or suffixed with "event" (or something similar). Furthermore, moveBalanceTransaction can be renamed to include the prefix or suffix "processing type".

andreibancioiu · 2023-05-22T13:35:21Z

process/transactionProcessor.go

+	if findIdentifierInLogs(tx, txFailed) {
+		return transaction.TxStatusFail
+	}
+	containsCompletion := findIdentifierInLogs(tx, txCompleted) || findIdentifierInLogs(tx, scDeploy)


Unfortunately, a transaction may have multiple txCompleted events, and finding the first ones do not indicate actual completion (e.g. think of smart contract calls that perform a few ESDT transfers within the SC execution, one of them intra shard, others cross shard). However, this should be sufficient for now (in my opinion).

I think this will be refactored in the upcoming VM v1.5. I would leave it as it is for now, as I do not have a better idea how to implement what you have enumerated :(

bogdan-rosianu · 2023-05-22T14:41:34Z

process/testdata/finishedFailedSCCall.json

  "initiallyPaidFee": "407005000000000",
  "fee": "407005000000000",
-  "chainID": "1",
+  "chainID": "T",


empty line at the end of the file

miiu96 · 2023-05-23T06:02:47Z

process/testdata/executingSCCall.json

@@ -0,0 +1,123 @@
+{


can you add also this transaction in your test data, it's a complex one that failed https://explorer.multiversx.com/transactions/a93ab7b43150786e2a9639137d3ba13c31273b2f82b9555b1fb2a76b3d711151

sure, will add

miiu96 · 2023-05-23T06:08:12Z

process/transactionProcessor.go

+	fieldsParam           = "?fields="
+	lastNonceParam        = "?last-nonce=true"
+	nonceGapsParam        = "?nonce-gaps=true"
+	txCompletedEvent      = "completedTxEvent"


can you use these constants from here https://github.com/multiversx/mx-chain-core-go/blob/16023cace5b78df86fbaa1e6a6b3778873f58c25/core/constants.go#L250
for the logs identifiers

also, take into consideration the log with the identifier internalVMErrors

great, will do

miiu96 · 2023-05-23T06:13:49Z

process/transactionProcessor.go

+	if findIdentifierInLogs(tx, txFailedEvent) {
+		return transaction.TxStatusFail
+	}
+	containsCompletion := findIdentifierInLogs(tx, txCompletedEvent) || findIdentifierInLogs(tx, scDeployEvent)


why do you search here for the log with the scDeploysEvent ?

Inspired from here: https://github.com/multiversx/mx-sdk-js-network-providers/blob/7bb49e6be570e581a92154aabbb63fb8db82eac4/src/transactionCompletionStrategy.ts#LL12C1-L12C1

miiu96 · 2023-05-24T07:02:56Z

process/testdata/finishedFailedComplexScenario3.json

+      "options": 0
+    }
+  ]
+}


empty line at the end of the file

miiu96 · 2023-05-24T07:03:04Z

process/testdata/finishedFailedComplexScenario2.json

+      "options": 0
+    }
+  ]
+}


add empty line

iulianpascalau · 2023-05-24T12:00:15Z

facade/mock/transactionProcessorStub.go

 	"math/big"

 	"github.com/multiversx/mx-chain-core-go/data/transaction"
 	"github.com/multiversx/mx-chain-proxy-go/data"
 )

+var errNotImplemented = errors.New("not implement")


not implemented *

done, thanks

bogdan-rosianu · 2023-05-24T12:20:39Z

process/mock/loggerStub.go

@@ -0,0 +1,96 @@
+package mock


I think that a logger mock and/or stub should be exported from the mx-chain-logger-go. similar to http vs http test:

"net/http" "net/http/httptest"

this way we don't have to redeclare these files everywhere

That's actually a bad habit. Interfaces & stubs should be replicated in each repo for maximum code reuse.
I could have used the mock from the core repo but I did not use it, specifically.

Mocks & other test components that have some logic inside.... yes, we might import them but that might prove a careless move in the long run, so I would avoid them

bogdan-rosianu · 2023-05-24T12:22:39Z

process/transactionProcessor.go

-func (tp *TransactionProcessor) GetProcessedTransactionStatus(txHash string, sender string) (string, error) {
-	tx, err := tp.getTransaction(txHash, sender, true)
+func (tp *TransactionProcessor) GetProcessedTransactionStatus(txHash string) (string, error) {
+	const withResult = true


withResults

bogdan-rosianu · 2023-05-24T12:24:35Z

process/transactionProcessor.go

+	if findIdentifierInLogs(logs, core.SignalErrorOperation) {
+		return true
+	}
+	if findIdentifierInLogs(logs, internalVMErrorsEventIdentifier) {


might switch the 2 checks. based on the test data, internal vm errors event seems to appear before the signal error one. just a slight improvement for early exits

bogdan-rosianu · 2023-05-24T12:26:52Z

process/transactionProcessor.go

@@ -468,6 +504,22 @@ func findIdentifierInLogs(tx *transaction.ApiTransactionResult, identifier strin
 	return false
 }

+func (tp *TransactionProcessor) gatherAllLogs(tx *transaction.ApiTransactionResult) ([]*transaction.ApiLogs, error) {
+	const withResult = true


withResults

andreibancioiu · 2023-05-24T12:56:50Z

api/groups/baseTransactionGroup.go

@@ -247,6 +248,22 @@ func (group *transactionGroup) getTransaction(c *gin.Context) {
 	shared.RespondWith(c, http.StatusOK, gin.H{"transaction": tx}, "", data.ReturnCodeSuccess)
 }

+func (group *transactionGroup) getProcessStatus(c *gin.Context) {


processingStatus vs. processStatus

actually I think it's better getProcessedTransactionStatus to align with the facade

iulianpascalau · 2023-06-22T10:09:38Z

process/testdata/finishedOKSCDeploy.json

+    "version": 1,
+    "options": 0
+  }
+}


add empty line

iulianpascalau · 2023-06-22T10:09:45Z

process/testdata/finishedOKRelayedTxWithSCCall.json

+      "options": 0
+    }
+  ]
+}


add empty line

iulianpascalau · 2023-06-22T10:09:54Z

process/testdata/finishedInvalidBuiltinFunction.json

+    "fee": "5852440000000000",
+    "options": 0
+  }
+}


add empty line

iulianpascalau · 2023-06-22T10:10:02Z

process/testdata/finishedFailedRelayedTxWithSCCall.json

+      "options": 0
+    }
+  ]
+}


add empty line

iulianpascalau · 2023-06-22T10:10:09Z

process/testdata/finishedFailedRelayedTxUnexecutable.json

+      "options": 0
+    }
+  ]
+}


add empty line

gabi-vuls

System test passed.

- added transaction processed status

8339380

andreibancioiu self-requested a review May 22, 2023 12:35

andreibancioiu assigned iulianpascalau May 22, 2023

bogdan-rosianu requested review from bogdan-rosianu and miiu96 May 22, 2023 12:43

bogdan-rosianu reviewed May 22, 2023

View reviewed changes

- fix after review: renaming + added TODO

372e6fd

andreibancioiu reviewed May 22, 2023

View reviewed changes

- fixes after review: reduced test .json file size & renaming

dcc419a

bogdan-rosianu reviewed May 22, 2023

View reviewed changes

- added empty line

d798499

andreibancioiu previously approved these changes May 22, 2023

View reviewed changes

miiu96 reviewed May 23, 2023

View reviewed changes

- refactored solution, added more scenarios

00ba495

iulianpascalau dismissed andreibancioiu’s stale review via 00ba495 May 23, 2023 15:25

miiu96 reviewed May 24, 2023

View reviewed changes

- added empty lines

c42eb51

miiu96 previously approved these changes May 24, 2023

View reviewed changes

- simplified solution

b23b884

iulianpascalau dismissed miiu96’s stale review via b23b884 May 24, 2023 11:58

iulianpascalau commented May 24, 2023

View reviewed changes

- fix after self review

5c71fea

bogdan-rosianu reviewed May 24, 2023

View reviewed changes

- fixes after review

7ee70f5

andreibancioiu previously approved these changes May 24, 2023

View reviewed changes

- rename the function

f46c807

iulianpascalau dismissed andreibancioiu’s stale review via f46c807 May 24, 2023 13:12

miiu96 previously approved these changes May 24, 2023

View reviewed changes

bogdan-rosianu previously approved these changes May 24, 2023

View reviewed changes

andreibancioiu previously approved these changes May 24, 2023

View reviewed changes

- added more test cases + fixes

d51b5e4

iulianpascalau dismissed stale reviews from andreibancioiu, bogdan-rosianu, and miiu96 via d51b5e4 May 30, 2023 15:32

- fixed test & removed TODO

ab9804f

iulianpascalau commented Jun 22, 2023

View reviewed changes

- fixes after self review, added empty lines

44d53d8

gabi-vuls approved these changes Jun 23, 2023

View reviewed changes

bogdan-rosianu approved these changes Jun 26, 2023

View reviewed changes

andreibancioiu approved these changes Jun 26, 2023

View reviewed changes

iulianpascalau merged commit 9ef2476 into rc/v1.5.0 Jun 26, 2023

iulianpascalau deleted the added-processed-tx-status branch June 26, 2023 09:28

This was referenced Jul 21, 2023

New transaction status for proxy multiversx/mx-sdk-js-network-providers#41

Merged

Adjust / improve ResultsParser to properly handle outcome of relayed transactions multiversx/mx-sdk-js-core#305

Open

andreibancioiu mentioned this pull request Aug 22, 2023

Check if isCompleted is undefined multiversx/mx-sdk-js-core#315

Merged

		"receiver": "erd1qqqqqqqqqqqqqpgq50dge6rrpcra4tp9hl57jl0893a4r2r72jpsk39rjj",
		"sender": "erd1nfqu46k8ffc2zkzme3dunh675sv69vnjp605c8pk77akk4f3e6kq5hegux",

Added transaction processed status #386

Added transaction processed status #386

Conversation

iulianpascalau commented May 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreibancioiu May 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miiu96 May 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabi-vuls left a comment

Choose a reason for hiding this comment

andreibancioiu May 22, 2023 •

edited

Loading

miiu96 May 23, 2023 •

edited

Loading