*: implement System.Blockchain.GetTransaction and System.Blockchain.GetBlock interops #1034

AnnaShaleva · 2020-06-09T11:14:04Z

codecov · 2020-06-09T11:15:49Z

Codecov Report

Merging #1034 into master will increase coverage by 0.31%.
The diff coverage is 77.27%.

@@            Coverage Diff             @@
##           master    #1034      +/-   ##
==========================================
+ Coverage   65.64%   65.95%   +0.31%     
==========================================
  Files         190      187       -3     
  Lines       15993    15945      -48     
==========================================
+ Hits        10498    10517      +19     
+ Misses       4981     4909      -72     
- Partials      514      519       +5

Impacted Files	Coverage Δ
pkg/core/interop_neo.go	`58.01% <ø> (-3.18%)`	⬇️
pkg/core/interops.go	`100.00% <ø> (ø)`
pkg/interop/blockchain/blockchain.go	`0.00% <0.00%> (ø)`
pkg/interop/engine/engine.go	`0.00% <0.00%> (ø)`
pkg/compiler/codegen.go	`90.00% <33.33%> (-0.20%)`	⬇️
pkg/core/interop_system.go	`51.82% <89.58%> (+11.70%)`	⬆️
pkg/core/gas_price.go	`100.00% <100.00%> (ø)`
pkg/core/transaction/attribute.go	`11.62% <0.00%> (+7.75%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ddcc35...8b7abd3. Read the comment docs.

fyrchik · 2020-06-09T12:20:38Z

pkg/core/interop_system.go

+	if index > height {
+		return false
+	}
+	return index+MaxTraceableBlocks > height


return index <= height && index + MaxTraceableBlocks > height ?

fyrchik · 2020-06-09T12:24:04Z

pkg/interop/block/block.go

-func GetTransactions(b Block) []transaction.Transaction {
-	return []transaction.Transaction{}
+func GetTransactions(b Block) []blockchain.Transaction {
+	return []blockchain.Transaction{}


Implementation of this and next interop should also be changed.

Oh, I see they were removed later. Probably worth removing before.

fyrchik · 2020-06-09T12:25:39Z

pkg/core/interop_system.go

@@ -145,6 +145,10 @@ func bcGetTransactionHeight(ic *interop.Context, v *vm.VM) error {
 	if err != nil {
 		return err


Should we return -1 here now?

For ErrKeyNotFound sure, we may have dropped it already and it's perfectly fine.

Changed to -1.

fyrchik · 2020-06-09T12:28:32Z

pkg/core/interop_system.go

+	}
+	block, err := ic.Chain.GetBlock(hash)
+	if err != nil || !isTraceableBlock(ic, block.Index) {
+		v.Estack().PushVal(stackitem.Null{})


We should return after this.

fyrchik · 2020-06-09T12:30:12Z

pkg/core/interop_system.go

+		v.Estack().PushVal(stackitem.Null{})
+	}
+	index := v.Estack().Pop().BigInt().Int64()
+	if index < 0 || index >= int64(len(block.Transactions)) {


C# implementation uses len-1 (and index+1 below). Probably 1st tx shouldn't be taken into account.

That's just to compensate for consensusDataHash, we don't have this problem.

That's true.

pkg/compiler/codegen.go

pkg/core/transaction/transaction.go

pkg/interop/blockchain/blockchain.go

roman-khimov · 2020-06-09T14:53:38Z

pkg/core/interop_system.go

+		v.Estack().PushVal(stackitem.Null{})
+	}
+	index := v.Estack().Pop().BigInt().Int64()
+	if index < 0 || index >= int64(len(block.Transactions)) {


That's just to compensate for consensusDataHash, we don't have this problem.

pkg/core/interop_system.go

roman-khimov · 2020-06-09T14:58:41Z

pkg/core/interop_system.go

+		return errors.New("wrong transaction index")
+	}
+	tx := block.Transactions[index]
+	v.Estack().PushVal(tx.ToStackItem())


We should put tx's hash here, not tx itself. And we should have a test for this interop also.

pkg/core/block/block.go

pkg/interop/blockchain/blockchain.go

We don't need them anymore.

closes #1023 Now we put on stack stackitem.Array instead of Interop, so we don't need old transaction-related interops anymore. Removed the following interops: System.Transaction.GetHash Neo.Transaction.GetAttributes Neo.Transaction.GetHash Neo.Transaction.GetWitnesses Neo.Attribute.GetData Neo.Attribute.GetUsage Also removed the following duplicated NEO interop: Neo.Blockchain.GetTransaction

Update System.Blockchain.GetTransactionHeight and removed Neo.Blockchain.GetTransactionHeight interop as we don't need it.

Removed Neo.Block.GetTransaction and System.Block.GetTransaction interops. These interops were replaced by new System.Blockchain.GetTransactionFromBlock interop.

closes #1025 Now we put on stack stackitem.Array instead of Interop, so we're able to use all available block properties without extra interop getters. Removed Neo.Blockchain.GetBlock interop as we don't need it anymore.

Updated System.Blockchain.GetBlock interop replaced the functionality of the following interops: System.Block.GetTransactions System.Block.GetTransactionCount Neo.Block.GetTransactions Neo.Block.GetTransactionsCount

obsolete

AnnaShaleva added the neo3 label Jun 9, 2020

AnnaShaleva requested review from fyrchik and roman-khimov June 9, 2020 11:14

AnnaShaleva self-assigned this Jun 9, 2020

fyrchik previously requested changes Jun 9, 2020

View reviewed changes

roman-khimov requested changes Jun 9, 2020

View reviewed changes

AnnaShaleva added 5 commits June 9, 2020 20:54

core: remove old AntShares interops

f2e3be6

We don't need them anymore.

core: update System.Blockchain.GetTransactionHeight interop

1e63ae4

Update System.Blockchain.GetTransactionHeight and removed Neo.Blockchain.GetTransactionHeight interop as we don't need it.

core: add System.Blockchain.GetTransactionFromBlock interop

d692de5

core: remove Block.GetTransaction interops

e2187c0

Removed Neo.Block.GetTransaction and System.Block.GetTransaction interops. These interops were replaced by new System.Blockchain.GetTransactionFromBlock interop.

AnnaShaleva force-pushed the neo3/interop/gettransaction branch from 9df8300 to 61d81f7 Compare June 9, 2020 20:18

AnnaShaleva added 2 commits June 9, 2020 23:24

core: update System.Blockchain.GetBlock interop

7a2d37c

closes #1025 Now we put on stack stackitem.Array instead of Interop, so we're able to use all available block properties without extra interop getters. Removed Neo.Blockchain.GetBlock interop as we don't need it anymore.

AnnaShaleva force-pushed the neo3/interop/gettransaction branch from 61d81f7 to 8b7abd3 Compare June 9, 2020 20:25

roman-khimov approved these changes Jun 9, 2020

View reviewed changes

roman-khimov merged commit 851e72f into master Jun 10, 2020

roman-khimov deleted the neo3/interop/gettransaction branch June 10, 2020 07:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: implement System.Blockchain.GetTransaction and System.Blockchain.GetBlock interops #1034

*: implement System.Blockchain.GetTransaction and System.Blockchain.GetBlock interops #1034

AnnaShaleva commented Jun 9, 2020

codecov bot commented Jun 9, 2020 •

edited

Loading

fyrchik Jun 9, 2020

fyrchik Jun 9, 2020

fyrchik Jun 9, 2020 •

edited

Loading

fyrchik Jun 9, 2020

roman-khimov Jun 9, 2020

AnnaShaleva Jun 9, 2020

fyrchik Jun 9, 2020

fyrchik Jun 9, 2020

roman-khimov Jun 9, 2020

AnnaShaleva Jun 9, 2020

roman-khimov Jun 9, 2020

roman-khimov Jun 9, 2020

		@@ -145,6 +145,10 @@ func bcGetTransactionHeight(ic interop.Context, v vm.VM) error {
		if err != nil {
		return err

*: implement System.Blockchain.GetTransaction and System.Blockchain.GetBlock interops #1034

*: implement System.Blockchain.GetTransaction and System.Blockchain.GetBlock interops #1034

Conversation

AnnaShaleva commented Jun 9, 2020

codecov bot commented Jun 9, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fyrchik Jun 9, 2020 • 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

codecov bot commented Jun 9, 2020 •

edited

Loading

fyrchik Jun 9, 2020 •

edited

Loading