From 8189ebe8dd674f871863c65b802e759223eaee48 Mon Sep 17 00:00:00 2001 From: Patrick O'Grady Date: Wed, 22 Jul 2020 16:09:51 -0700 Subject: [PATCH 1/5] Write intent checks --- parser/intent.go | 148 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 148 insertions(+) create mode 100644 parser/intent.go diff --git a/parser/intent.go b/parser/intent.go new file mode 100644 index 00000000..583adf20 --- /dev/null +++ b/parser/intent.go @@ -0,0 +1,148 @@ +package parser + +import ( + "context" + "fmt" + "log" + + "github.com/coinbase/rosetta-sdk-go/types" +) + +// ExpectedOperation returns an error if an observed operation +// differs from the intended operation. An operation is considered +// to be different from the intent if the AccountIdentifier, +// Amount, or Type has changed. +func ExpectedOperation(intent *types.Operation, observed *types.Operation) error { + if types.Hash(intent.Account) != types.Hash(observed.Account) { + return fmt.Errorf( + "intended account %s did not match observed account %s", + types.PrettyPrintStruct(intent.Account), + types.PrettyPrintStruct(observed.Account), + ) + } + + if types.Hash(intent.Amount) != types.Hash(observed.Amount) { + return fmt.Errorf( + "intended amount %s did not match observed amount %s", + types.PrettyPrintStruct(intent.Amount), + types.PrettyPrintStruct(observed.Amount), + ) + } + + if intent.Type != observed.Type { + return fmt.Errorf( + "intended type %s did not match observed type %s", + intent.Type, + observed.Type, + ) + } + + return nil +} + +// ExpectedOperations returns an error if a slice of intended +// operations differ from observed operations. Optionally, +// it is possible to error if any extra observed opertions +// are found. +func ExpectedOperations( + ctx context.Context, + intent []*types.Operation, + observed []*types.Operation, + errExtra bool, +) error { + matches := map[int]int{} // mapping from intent -> observed + unmatched := []int{} // observed + + for o, obs := range observed { + foundMatch := false + for i, in := range intent { + if _, exists := matches[i]; exists { + continue + } + + if ExpectedOperation(in, obs) == nil { + matches[i] = o + foundMatch = true + break + } + } + + if !foundMatch { + if errExtra { + return fmt.Errorf( + "found extra operation %s", + types.PrettyPrintStruct(o), + ) + } + unmatched = append(unmatched, o) + } + } + + if len(matches) != len(intent) { + for i := 0; i < len(intent); i++ { + if _, exists := matches[i]; !exists { + return fmt.Errorf( + "could not find operation with intent %s in observed", + types.PrettyPrintStruct(intent[i]), + ) + } + } + } + + for _, extra := range unmatched { + log.Printf("found extra operation %s\n", types.PrettyPrintStruct(observed[extra])) + } + + return nil +} + +// ExpectedSigners returns an error if a slice of SigningPayload +// has different signers than what was observed (typically populated +// using the signers returned from parsing a transaction). +func ExpectedSigners(intent []*types.SigningPayload, observed []string) error { + matches := map[int]int{} // requested -> observed + unmatched := []int{} // observed + for o, observed := range observed { + foundMatch := false + for r, req := range intent { + if _, exists := matches[r]; exists { + continue + } + + if req.Address == observed { + matches[r] = o + foundMatch = true + break + } + } + + if !foundMatch { + unmatched = append(unmatched, o) + } + } + + if len(matches) != len(intent) { + for i := 0; i < len(intent); i++ { + if _, exists := matches[i]; !exists { + return fmt.Errorf( + "could not find signer with address %s in observed", + types.PrettyPrintStruct(intent[i]), + ) + } + } + } + + if len(unmatched) != 0 { + unexpected := []string{} + for _, m := range unmatched { + unexpected = append(unexpected, observed[m]) + } + + return fmt.Errorf( + "found unexpected signers: %s", + types.PrettyPrintStruct(unexpected), + ) + } + + return nil +} From e4d138e05d801e316d46935ce40d96661843ccf3 Mon Sep 17 00:00:00 2001 From: Patrick O'Grady Date: Wed, 22 Jul 2020 16:16:46 -0700 Subject: [PATCH 2/5] Add test for ExpectedOperation --- parser/intent_test.go | 160 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 160 insertions(+) create mode 100644 parser/intent_test.go diff --git a/parser/intent_test.go b/parser/intent_test.go new file mode 100644 index 00000000..4757773c --- /dev/null +++ b/parser/intent_test.go @@ -0,0 +1,160 @@ +package parser + +import ( + "testing" + + "github.com/coinbase/rosetta-sdk-go/types" + "github.com/stretchr/testify/assert" +) + +func TestExpectedOperation(t *testing.T) { + var tests = map[string]struct { + intent *types.Operation + observed *types.Operation + + err bool + }{ + "simple match": { + intent: &types.Operation{ + OperationIdentifier: &types.OperationIdentifier{ + Index: 1, + }, + Type: "transfer", + Account: &types.AccountIdentifier{ + Address: "addr1", + }, + Amount: &types.Amount{ + Value: "100", + }, + }, + observed: &types.Operation{ + OperationIdentifier: &types.OperationIdentifier{ + Index: 3, + }, + RelatedOperations: []*types.OperationIdentifier{ + { + Index: 2, + }, + }, + Status: "success", + Type: "transfer", + Account: &types.AccountIdentifier{ + Address: "addr1", + }, + Amount: &types.Amount{ + Value: "100", + }, + }, + }, + "account mismatch": { + intent: &types.Operation{ + OperationIdentifier: &types.OperationIdentifier{ + Index: 1, + }, + Type: "transfer", + Account: &types.AccountIdentifier{ + Address: "addr1", + }, + Amount: &types.Amount{ + Value: "100", + }, + }, + observed: &types.Operation{ + OperationIdentifier: &types.OperationIdentifier{ + Index: 3, + }, + RelatedOperations: []*types.OperationIdentifier{ + { + Index: 2, + }, + }, + Status: "success", + Type: "transfer", + Account: &types.AccountIdentifier{ + Address: "addr2", + }, + Amount: &types.Amount{ + Value: "100", + }, + }, + err: true, + }, + "amount mismatch": { + intent: &types.Operation{ + OperationIdentifier: &types.OperationIdentifier{ + Index: 1, + }, + Type: "transfer", + Account: &types.AccountIdentifier{ + Address: "addr1", + }, + Amount: &types.Amount{ + Value: "100", + }, + }, + observed: &types.Operation{ + OperationIdentifier: &types.OperationIdentifier{ + Index: 3, + }, + RelatedOperations: []*types.OperationIdentifier{ + { + Index: 2, + }, + }, + Status: "success", + Type: "transfer", + Account: &types.AccountIdentifier{ + Address: "addr1", + }, + Amount: &types.Amount{ + Value: "150", + }, + }, + err: true, + }, + "type mismatch": { + intent: &types.Operation{ + OperationIdentifier: &types.OperationIdentifier{ + Index: 1, + }, + Type: "transfer", + Account: &types.AccountIdentifier{ + Address: "addr1", + }, + Amount: &types.Amount{ + Value: "100", + }, + }, + observed: &types.Operation{ + OperationIdentifier: &types.OperationIdentifier{ + Index: 3, + }, + RelatedOperations: []*types.OperationIdentifier{ + { + Index: 2, + }, + }, + Status: "success", + Type: "reward", + Account: &types.AccountIdentifier{ + Address: "addr1", + }, + Amount: &types.Amount{ + Value: "100", + }, + }, + err: true, + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + err := ExpectedOperation(test.intent, test.observed) + if test.err { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} From 1fae00c5142c72555abc1b9b2c61080ba0626c71 Mon Sep 17 00:00:00 2001 From: Patrick O'Grady Date: Wed, 22 Jul 2020 16:23:11 -0700 Subject: [PATCH 3/5] Add test for ExpectedOperations --- parser/intent.go | 2 - parser/intent_test.go | 183 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 183 insertions(+), 2 deletions(-) diff --git a/parser/intent.go b/parser/intent.go index 583adf20..bc46fc28 100644 --- a/parser/intent.go +++ b/parser/intent.go @@ -1,7 +1,6 @@ package parser import ( - "context" "fmt" "log" @@ -45,7 +44,6 @@ func ExpectedOperation(intent *types.Operation, observed *types.Operation) error // it is possible to error if any extra observed opertions // are found. func ExpectedOperations( - ctx context.Context, intent []*types.Operation, observed []*types.Operation, errExtra bool, diff --git a/parser/intent_test.go b/parser/intent_test.go index 4757773c..e9233806 100644 --- a/parser/intent_test.go +++ b/parser/intent_test.go @@ -158,3 +158,186 @@ func TestExpectedOperation(t *testing.T) { }) } } + +func TestExpectedOperations(t *testing.T) { + var tests = map[string]struct { + intent []*types.Operation + observed []*types.Operation + errExtra bool + + err bool + }{ + "simple match": { + intent: []*types.Operation{ + { + OperationIdentifier: &types.OperationIdentifier{ + Index: 1, + }, + Type: "transfer", + Account: &types.AccountIdentifier{ + Address: "addr1", + }, + Amount: &types.Amount{ + Value: "100", + }, + }, + { + OperationIdentifier: &types.OperationIdentifier{ + Index: 5, + }, + Type: "fee", + Account: &types.AccountIdentifier{ + Address: "addr2", + }, + Amount: &types.Amount{ + Value: "50", + }, + }, + }, + observed: []*types.Operation{ + { + OperationIdentifier: &types.OperationIdentifier{ + Index: 2, + }, + Status: "success", + Type: "fee", + Account: &types.AccountIdentifier{ + Address: "addr2", + }, + Amount: &types.Amount{ + Value: "50", + }, + }, + { + OperationIdentifier: &types.OperationIdentifier{ + Index: 3, + }, + RelatedOperations: []*types.OperationIdentifier{ + { + Index: 2, + }, + }, + Status: "success", + Type: "transfer", + Account: &types.AccountIdentifier{ + Address: "addr1", + }, + Amount: &types.Amount{ + Value: "100", + }, + }, + }, + }, + "err extra": { + intent: []*types.Operation{ + { + OperationIdentifier: &types.OperationIdentifier{ + Index: 1, + }, + Type: "transfer", + Account: &types.AccountIdentifier{ + Address: "addr1", + }, + Amount: &types.Amount{ + Value: "100", + }, + }, + }, + observed: []*types.Operation{ + { + OperationIdentifier: &types.OperationIdentifier{ + Index: 2, + }, + Status: "success", + Type: "fee", + Account: &types.AccountIdentifier{ + Address: "addr2", + }, + Amount: &types.Amount{ + Value: "100", + }, + }, + { + OperationIdentifier: &types.OperationIdentifier{ + Index: 3, + }, + RelatedOperations: []*types.OperationIdentifier{ + { + Index: 2, + }, + }, + Status: "success", + Type: "transfer", + Account: &types.AccountIdentifier{ + Address: "addr1", + }, + Amount: &types.Amount{ + Value: "100", + }, + }, + }, + errExtra: true, + err: true, + }, + "missing match": { + intent: []*types.Operation{ + { + OperationIdentifier: &types.OperationIdentifier{ + Index: 1, + }, + Type: "transfer", + Account: &types.AccountIdentifier{ + Address: "addr1", + }, + Amount: &types.Amount{ + Value: "100", + }, + }, + { + OperationIdentifier: &types.OperationIdentifier{ + Index: 5, + }, + Type: "fee", + Account: &types.AccountIdentifier{ + Address: "addr2", + }, + Amount: &types.Amount{ + Value: "50", + }, + }, + }, + observed: []*types.Operation{ + { + OperationIdentifier: &types.OperationIdentifier{ + Index: 3, + }, + RelatedOperations: []*types.OperationIdentifier{ + { + Index: 2, + }, + }, + Status: "success", + Type: "transfer", + Account: &types.AccountIdentifier{ + Address: "addr1", + }, + Amount: &types.Amount{ + Value: "100", + }, + }, + }, + err: true, + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + err := ExpectedOperations(test.intent, test.observed, test.errExtra) + if test.err { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} From 4b59e17251f0460d2c731d7dc43428b07f3d7994 Mon Sep 17 00:00:00 2001 From: Patrick O'Grady Date: Wed, 22 Jul 2020 17:32:05 -0700 Subject: [PATCH 4/5] Add test for ExpectedSignatures --- parser/intent.go | 86 ++++++++++++++++++++----------------------- parser/intent_test.go | 86 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 126 insertions(+), 46 deletions(-) diff --git a/parser/intent.go b/parser/intent.go index bc46fc28..5b4db0e4 100644 --- a/parser/intent.go +++ b/parser/intent.go @@ -4,6 +4,7 @@ import ( "fmt" "log" + "github.com/coinbase/rosetta-sdk-go/asserter" "github.com/coinbase/rosetta-sdk-go/types" ) @@ -48,10 +49,10 @@ func ExpectedOperations( observed []*types.Operation, errExtra bool, ) error { - matches := map[int]int{} // mapping from intent -> observed - unmatched := []int{} // observed + matches := make(map[int]struct{}) + unmatched := []*types.Operation{} - for o, obs := range observed { + for _, obs := range observed { foundMatch := false for i, in := range intent { if _, exists := matches[i]; exists { @@ -59,7 +60,7 @@ func ExpectedOperations( } if ExpectedOperation(in, obs) == nil { - matches[i] = o + matches[i] = struct{}{} foundMatch = true break } @@ -69,26 +70,24 @@ func ExpectedOperations( if errExtra { return fmt.Errorf( "found extra operation %s", - types.PrettyPrintStruct(o), + types.PrettyPrintStruct(obs), ) } - unmatched = append(unmatched, o) + unmatched = append(unmatched, obs) } } - if len(matches) != len(intent) { - for i := 0; i < len(intent); i++ { - if _, exists := matches[i]; !exists { - return fmt.Errorf( - "could not find operation with intent %s in observed", - types.PrettyPrintStruct(intent[i]), - ) - } + for i := 0; i < len(intent); i++ { + if _, exists := matches[i]; !exists { + return fmt.Errorf( + "could not find operation with intent %s in observed", + types.PrettyPrintStruct(intent[i]), + ) } } - for _, extra := range unmatched { - log.Printf("found extra operation %s\n", types.PrettyPrintStruct(observed[extra])) + if len(unmatched) > 0 { + log.Printf("found extra operations: %s\n", types.PrettyPrintStruct(unmatched)) } return nil @@ -98,47 +97,42 @@ func ExpectedOperations( // has different signers than what was observed (typically populated // using the signers returned from parsing a transaction). func ExpectedSigners(intent []*types.SigningPayload, observed []string) error { - matches := map[int]int{} // requested -> observed - unmatched := []int{} // observed - for o, observed := range observed { - foundMatch := false - for r, req := range intent { - if _, exists := matches[r]; exists { - continue - } + // De-duplicate required signers (ex: multiple UTXOs from same address) + intendedSigners := make(map[string]struct{}) + for _, payload := range intent { + intendedSigners[payload.Address] = struct{}{} + } - if req.Address == observed { - matches[r] = o - foundMatch = true - break - } - } + if err := asserter.StringArray("observed signers", observed); err != nil { + return fmt.Errorf("%w: found duplicate signer", err) + } - if !foundMatch { - unmatched = append(unmatched, o) + // Could exist here if len(intent) != len(observed) but + // more useful to print out a detailed error message. + seenSigners := make(map[string]struct{}) + unmatched := []string{} // observed + for _, signer := range observed { + _, exists := intendedSigners[signer] + if !exists { + unmatched = append(unmatched, signer) + } else { + seenSigners[signer] = struct{}{} } } - if len(matches) != len(intent) { - for i := 0; i < len(intent); i++ { - if _, exists := matches[i]; !exists { - return fmt.Errorf( - "could not find signer with address %s in observed", - types.PrettyPrintStruct(intent[i]), - ) - } + for k := range intendedSigners { + if _, exists := seenSigners[k]; !exists { + return fmt.Errorf( + "could not find match for intended signer %s", + k, + ) } } if len(unmatched) != 0 { - unexpected := []string{} - for _, m := range unmatched { - unexpected = append(unexpected, observed[m]) - } - return fmt.Errorf( "found unexpected signers: %s", - types.PrettyPrintStruct(unexpected), + types.PrettyPrintStruct(unmatched), ) } diff --git a/parser/intent_test.go b/parser/intent_test.go index e9233806..4d5358ec 100644 --- a/parser/intent_test.go +++ b/parser/intent_test.go @@ -341,3 +341,89 @@ func TestExpectedOperations(t *testing.T) { }) } } + +func TestExpectedSigners(t *testing.T) { + var tests = map[string]struct { + intent []*types.SigningPayload + observed []string + + err bool + }{ + "simple match": { + intent: []*types.SigningPayload{ + { + Address: "addr1", + }, + { + Address: "addr2", + }, + { + Address: "addr2", + }, + }, + observed: []string{ + "addr1", + "addr2", + }, + }, + "duplicate observed signers": { + intent: []*types.SigningPayload{ + { + Address: "addr1", + }, + { + Address: "addr2", + }, + { + Address: "addr2", + }, + }, + observed: []string{ + "addr1", + "addr2", + "addr2", + }, + err: true, + }, + "missing observed signer": { + intent: []*types.SigningPayload{ + { + Address: "addr1", + }, + { + Address: "addr2", + }, + { + Address: "addr2", + }, + }, + observed: []string{ + "addr1", + }, + err: true, + }, + "extra observed signer": { + intent: []*types.SigningPayload{ + { + Address: "addr1", + }, + }, + observed: []string{ + "addr1", + "addr2", + }, + err: true, + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + err := ExpectedSigners(test.intent, test.observed) + if test.err { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} From 07c685a028f51f429b959920926fb261adfbe181 Mon Sep 17 00:00:00 2001 From: Patrick O'Grady Date: Wed, 22 Jul 2020 17:33:15 -0700 Subject: [PATCH 5/5] Nits --- parser/intent.go | 14 ++++++++++++++ parser/intent_test.go | 14 ++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/parser/intent.go b/parser/intent.go index 5b4db0e4..51d5e346 100644 --- a/parser/intent.go +++ b/parser/intent.go @@ -1,3 +1,17 @@ +// Copyright 2020 Coinbase, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package parser import ( diff --git a/parser/intent_test.go b/parser/intent_test.go index 4d5358ec..ab678ced 100644 --- a/parser/intent_test.go +++ b/parser/intent_test.go @@ -1,3 +1,17 @@ +// Copyright 2020 Coinbase, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package parser import (