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

Get transaction with status api route #1969

Merged
merged 11 commits into from Jun 24, 2020
Merged

Conversation

miiu96
Copy link
Contributor

@miiu96 miiu96 commented Jun 17, 2020

Removed api route /transaction/:hash/status because a single observer cannot provided a correct status of a transaction (the case when a transaction is cross shard)

Modified api route /transaction/:hash to return also transaction status from view of a obsever (is not 100% correct because an observer from shard 0 cannot know what is the status of a transaction from shard 1). Processing of a correct status of a transaction will be done on proxy application.

@miiu96 miiu96 marked this pull request as ready for review June 18, 2020 06:47
@miiu96 miiu96 changed the title Get transaction with status api route [WIP] Get transaction with status api route Jun 18, 2020
Copy link
Collaborator

@andreibancioiu andreibancioiu left a comment

Choose a reason for hiding this comment

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

👍

node/nodeTransactions.go Outdated Show resolved Hide resolved
core/constants.go Outdated Show resolved Hide resolved
}, nil
}

func (n *Node) computeTransactionStatus(tx data.TransactionHandler, isInPool bool) string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can return TransactionStatus here, and perhaps also use this type for the new tx field - and let the marshelizer do its job (optional - and hopefully it will work).

Copy link
Contributor

Choose a reason for hiding this comment

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

json marshaler will be happy with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes maybe json marshalizer will be happy with that but if we change core.TransactionStatus in a more complex structure that contains more than a single 'string' field will not work anymore. if you strongly suggest that ,i will change... wait for your answer.

node/nodeTransactions.go Outdated Show resolved Hide resolved
node/nodeTransactions.go Show resolved Hide resolved
node/nodeTransactions.go Outdated Show resolved Hide resolved
node/nodeTransactions.go Outdated Show resolved Hide resolved
@iulianpascalau iulianpascalau self-requested a review June 18, 2020 18:07
api/transaction/routes.go Show resolved Hide resolved
api/transaction/routes.go Show resolved Hide resolved
core/constants.go Outdated Show resolved Hide resolved
}, nil
}

func (n *Node) computeTransactionStatus(tx data.TransactionHandler, isInPool bool) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

json marshaler will be happy with that.

node/nodeTransactions_test.go Show resolved Hide resolved
@@ -294,6 +296,97 @@ func TestNode_GetTransaction_ShouldNotFindAndReturnUnknown(t *testing.T) {
assert.Error(t, err)
}

func TestNode_ComputeTransactionStatusAllBranches(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can remove the suffix "AllBranches" perhaps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

rwdTxCrossShard := &rewardTx.RewardTx{RcvAddr: shardZeroAddr}
normalTxIntraShard := &transaction.Transaction{RcvAddr: shardZeroAddr, SndAddr: shardZeroAddr}
normalTxCrossShard := &transaction.Transaction{RcvAddr: shardOneAddr, SndAddr: shardZeroAddr}
unsignedTxIntraShard := &smartContractResult.SmartContractResult{RcvAddr: shardOneAddr, SndAddr: shardZeroAddr}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This one is cross shard. Also add another one for intra-shard :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

node/nodeTransactions_test.go Show resolved Hide resolved
txStatus = n.ComputeTransactionStatus(unsignedTxIntraShard, true)
assert.Equal(t, core.TxStatusReceived, txStatus)

// cross shard scr in storage source shard
Copy link
Collaborator

Choose a reason for hiding this comment

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

Cross-shard, on destination, for rewards - not covered. But the test is sufficient though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah i think is sufficient because all branches are covered.

Copy link
Contributor

@LucianMincu LucianMincu left a comment

Choose a reason for hiding this comment

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

System tests passed.

@LucianMincu LucianMincu merged commit 8add3ae into release-candidate Jun 24, 2020
@LucianMincu LucianMincu deleted the get-tx-api-route branch June 24, 2020 23:07
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