core: move transaction sender to signers #1244

AnnaShaleva · 2020-07-30T11:33:45Z

fyrchik

I think every check used in DecodeBinary should also be added to UnmarshalJSON, because if there are any assumptions we expect for Transaction to hold (e.g. non-empty Signers array), we should make corresponding checks everytime we parse some external input.

pkg/core/transaction/witness_scope.go

cli/smartcontract/smart_contract.go

fyrchik · 2020-07-30T12:45:34Z

pkg/core/transaction/transaction.go

-		for j := i + 1; j < len(t.Cosigners); j++ {
-			if t.Cosigners[i].Account.Equals(t.Cosigners[j].Account) {
+		for j := i + 1; j < len(t.Signers); j++ {
+			if t.Signers[i].Account.Equals(t.Signers[j].Account) {
 				br.Err = errors.New("transaction cosigners should be unique")


cosigners -> signers ?

pkg/core/transaction/transaction.go

roman-khimov · 2020-08-03T16:37:29Z

pkg/rpc/server/subscription.go

@@ -58,12 +58,12 @@ func (f *feed) Matches(r *response.Notification) bool {
 	case response.TransactionEventID:
 		filt := f.filter.(request.TxFilter)
 		tx := r.Payload[0].(*transaction.Transaction)
-		senderOK := filt.Sender == nil || tx.Sender.Equals(*filt.Sender)
+		senderOK := filt.Sender == nil || tx.Sender().Equals(*filt.Sender)
 		cosignerOK := true
 		if filt.Cosigner != nil {


It should be filt.Signer now with the respective range tx.Signers semantics. Client and documentation are to be updated too.

Did you mean that we should remove filt.Sender or just rename filt.Cosigner and allow to use the sender as filt.Signer?
Don't we want to distinguish sender and cosigners in subscriptions? I thought that it might be useful, because we don't have scopes in subscriptions and there's no need to subscribe to sender in filt.Signer when you can just subscribe to filt.Sender.

rename filt.Cosigner and allow to use the sender as filt.Signer

Like this one.

Don't we want to distinguish sender and cosigners in subscriptions?

Well, I might be wrong but I see two main use cases --- either you care about some address being a sender or you care about some address being one of signers (including being sender). Filtering for signer address which is not a sender seems a bit suspicious. Maybe there is a use case for it too, but I can't see any at the moment.

roman-khimov · 2020-08-03T16:40:29Z

pkg/rpc/server/server.go

 		if err != nil {
 			return nil, response.ErrInvalidParams
 		}
-		tx.Cosigners = cosigners
+		tx.Signers = signers


I think we have to initialize tx.Signers with some stub sender anyway, otherwise some code working with senders can fail.

roman-khimov · 2020-08-03T16:41:22Z

cli/smartcontract/smart_contract.go

-   arguments).
+   arguments and signers (sender is not included by default). If no method is given
+   "" is passed to the script, if no arguments are given, an empty array is 
+   passed, if no signers are given no array is passed. All of the given 


And when there are some signers, the first one of them is going to be the sender, it should be specified here too.

roman-khimov · 2020-08-03T16:42:52Z

cli/smartcontract/smart_contract.go

+   Signers represent a set of Uint160 hashes with witness scopes and are used
+   to verify hashes in System.Runtime.CheckWitness syscall. To specify signers
+   use signer[:scope] syntax where
+    * 'signer' is hex-encoded 160 bit (20 byte) LE value of signer's address,
                 which could have '0x' prefix.
    * 'scope' is a comma-separated set of cosigner's scopes, which could be:


We're missing FeeOnly scope here.

AnnaShaleva · 2020-08-04T12:12:09Z

@roman-khimov please, look #1265 first.

codecov · 2020-08-04T12:14:18Z

Codecov Report

Merging #1244 into master will decrease coverage by 0.13%.
The diff coverage is 46.34%.

@@            Coverage Diff             @@
##           master    #1244      +/-   ##
==========================================
- Coverage   67.76%   67.62%   -0.14%     
==========================================
  Files         215      215              
  Lines       18297    18328      +31     
==========================================
- Hits        12399    12395       -4     
- Misses       5240     5267      +27     
- Partials      658      666       +8

Impacted Files	Coverage Δ
cli/smartcontract/smart_contract.go	`0.00% <0.00%> (ø)`
pkg/rpc/client/nep5.go	`3.77% <0.00%> (+0.03%)`	⬆️
pkg/rpc/client/rpc.go	`77.51% <22.58%> (-2.61%)`	⬇️
pkg/core/transaction/signer.go	`50.00% <25.00%> (ø)`
pkg/core/transaction/transaction.go	`77.77% <48.48%> (-1.24%)`	⬇️
pkg/core/transaction/witness_scope_string.go	`52.63% <50.00%> (+2.63%)`	⬆️
pkg/rpc/server/server.go	`78.70% <75.00%> (-0.15%)`	⬇️
pkg/core/blockchain.go	`66.85% <100.00%> (-0.61%)`	⬇️
pkg/core/interop/runtime/witness.go	`45.09% <100.00%> (ø)`
pkg/core/interop_system.go	`64.61% <100.00%> (ø)`
... and 10 more

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 7865bc5...ba08a9b. Read the comment docs.

roman-khimov · 2020-08-04T12:33:43Z

pkg/core/native_policy_test.go

@@ -5,6 +5,8 @@ import (
 	"sort"
 	"testing"

+	"github.com/stretchr/testify/require"
+


Useless change.

roman-khimov · 2020-08-04T12:34:43Z

pkg/core/transaction/transaction.go

+// in the transaction's signers list.
+func (t *Transaction) Sender() util.Uint160 {
+	if len(t.Signers) == 0 {
+		panic("transaction do not have signers")


does not or has no

Closes #1184 Ported changes from neo-project/neo#1752

We should perform the same checks during UnmarshalJSON.

AnnaShaleva requested a review from fyrchik July 30, 2020 11:33

fyrchik suggested changes Jul 30, 2020

View reviewed changes

AnnaShaleva requested a review from fyrchik July 31, 2020 08:36

fyrchik approved these changes Jul 31, 2020

View reviewed changes

pkg/core/transaction/transaction.go Outdated Show resolved Hide resolved

pkg/core/transaction/transaction.go Outdated Show resolved Hide resolved

AnnaShaleva force-pushed the transaction/signers branch from c6afaa0 to fc02e41 Compare July 31, 2020 09:20

roman-khimov requested changes Aug 3, 2020

View reviewed changes

AnnaShaleva force-pushed the transaction/signers branch 2 times, most recently from 121fe9c to 72a41a1 Compare August 4, 2020 10:44

AnnaShaleva added 2 commits August 4, 2020 15:08

core: add scopes check to cosigners in DecodeBinary

75f1c2d

core: add FeeOnly witness scope

8697582

AnnaShaleva force-pushed the transaction/signers branch from 72a41a1 to 6141a4f Compare August 4, 2020 12:09

roman-khimov reviewed Aug 4, 2020

View reviewed changes

AnnaShaleva added 3 commits August 4, 2020 17:33

core: move transaction's sender to cosigners

90825ef

Closes #1184 Ported changes from neo-project/neo#1752

core: remove unnecessary comment

ead6bdb

core: move all transaction.Decode checks to separate method

ba08a9b

We should perform the same checks during UnmarshalJSON.

AnnaShaleva force-pushed the transaction/signers branch from 6141a4f to ba08a9b Compare August 4, 2020 14:34

AnnaShaleva requested a review from roman-khimov August 4, 2020 14:41

roman-khimov approved these changes Aug 4, 2020

View reviewed changes

roman-khimov merged commit 058a26b into master Aug 4, 2020

roman-khimov deleted the transaction/signers branch August 4, 2020 16:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: move transaction sender to signers #1244

core: move transaction sender to signers #1244

AnnaShaleva commented Jul 30, 2020

fyrchik left a comment

fyrchik Jul 30, 2020

roman-khimov Aug 3, 2020

AnnaShaleva Aug 4, 2020

roman-khimov Aug 4, 2020

roman-khimov Aug 3, 2020

roman-khimov Aug 3, 2020

roman-khimov Aug 3, 2020

AnnaShaleva commented Aug 4, 2020

codecov bot commented Aug 4, 2020 •

edited

Loading

roman-khimov Aug 4, 2020

roman-khimov Aug 4, 2020

core: move transaction sender to signers #1244

core: move transaction sender to signers #1244

Conversation

AnnaShaleva commented Jul 30, 2020

fyrchik left a 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

AnnaShaleva commented Aug 4, 2020

codecov bot commented Aug 4, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 4, 2020 •

edited

Loading