From a121375505e13585630bb08f45e8a7477d28fe90 Mon Sep 17 00:00:00 2001 From: Patrick O'Grady Date: Mon, 11 May 2020 19:41:15 -0700 Subject: [PATCH 1/3] Ensure Operation Groups are sorted --- parser/group_operations.go | 40 ++++++++++++++++---- parser/group_operations_test.go | 65 +++++++++++++++++++++++++++++++++ 2 files changed, 97 insertions(+), 8 deletions(-) diff --git a/parser/group_operations.go b/parser/group_operations.go index 32d34203..5cad2083 100644 --- a/parser/group_operations.go +++ b/parser/group_operations.go @@ -15,6 +15,8 @@ package parser import ( + "sort" + "github.com/coinbase/rosetta-sdk-go/asserter" "github.com/coinbase/rosetta-sdk-go/types" ) @@ -66,11 +68,36 @@ func addOperationToGroup( } } +func sortOperationGroups(opLen int, opGroups map[int]*OperationGroup) []*OperationGroup { + sliceGroups := []*OperationGroup{} + + // Golang map ordering is non-deterministic. + // Return groups sorted by lowest op in group + for i := 0; i < opLen; i++ { + v, ok := opGroups[i] + if !ok { + continue + } + + // Sort all operations by index in a group + sort.Slice(v.Operations, func(i, j int) bool { + return v.Operations[i].OperationIdentifier.Index < v.Operations[j].OperationIdentifier.Index + }) + + sliceGroups = append(sliceGroups, v) + } + + return sliceGroups +} + // GroupOperations parses all of a transaction's opertations and returns a slice // of each group of related operations. This should ONLY be called on operations // that have already been asserted for correctness. Assertion ensures there are // no duplicate operation indexes, operations are sorted, and that operations // only reference operations with an index less than theirs. +// +// OperationGroups are returned in ascending order based on the lowest +// operation index in the group. func GroupOperations(transaction *types.Transaction) []*OperationGroup { ops := transaction.Operations opGroups := map[int]*OperationGroup{} // using a map makes group merges much easier @@ -103,6 +130,10 @@ func GroupOperations(transaction *types.Transaction) []*OperationGroup { } } + // Ensure first index is lowest because all other groups + // will be merged into it. + sort.Ints(groupsToMerge) + mergedGroupIndex := groupsToMerge[0] mergedGroup := opGroups[mergedGroupIndex] @@ -123,12 +154,5 @@ func GroupOperations(transaction *types.Transaction) []*OperationGroup { } } - return func() []*OperationGroup { - sliceGroups := []*OperationGroup{} - for _, v := range opGroups { - sliceGroups = append(sliceGroups, v) - } - - return sliceGroups - }() + return sortOperationGroups(len(ops), opGroups) } diff --git a/parser/group_operations_test.go b/parser/group_operations_test.go index 6c84449a..4e120dd0 100644 --- a/parser/group_operations_test.go +++ b/parser/group_operations_test.go @@ -22,6 +22,71 @@ import ( "github.com/stretchr/testify/assert" ) +func TestSortOperationGroups(t *testing.T) { + m := map[int]*OperationGroup{ + 2: { + Operations: []*types.Operation{ + { + OperationIdentifier: &types.OperationIdentifier{ + Index: 2, + }, + }, + }, + }, + 0: { + Operations: []*types.Operation{ + { + OperationIdentifier: &types.OperationIdentifier{ + Index: 1, + }, + RelatedOperations: []*types.OperationIdentifier{ + { + Index: 0, + }, + }, + }, + { + OperationIdentifier: &types.OperationIdentifier{ + Index: 0, + }, + }, + }, + }, + } + + sortedGroups := sortOperationGroups(3, m) + assert.Equal(t, []*OperationGroup{ + { + Operations: []*types.Operation{ + { + OperationIdentifier: &types.OperationIdentifier{ + Index: 0, + }, + }, + { + OperationIdentifier: &types.OperationIdentifier{ + Index: 1, + }, + RelatedOperations: []*types.OperationIdentifier{ + { + Index: 0, + }, + }, + }, + }, + }, + { + Operations: []*types.Operation{ + { + OperationIdentifier: &types.OperationIdentifier{ + Index: 2, + }, + }, + }, + }, + }, sortedGroups) +} + func TestGroupOperations(t *testing.T) { var tests = map[string]struct { transaction *types.Transaction From a0d2749bbc36dcd85a3171224a0dca39fc035f21 Mon Sep 17 00:00:00 2001 From: Patrick O'Grady Date: Mon, 11 May 2020 21:25:13 -0700 Subject: [PATCH 2/3] nits --- parser/group_operations.go | 18 +++++++--- parser/group_operations_test.go | 58 ++++++++++++++++++++++++++++++++- 2 files changed, 70 insertions(+), 6 deletions(-) diff --git a/parser/group_operations.go b/parser/group_operations.go index 5cad2083..f5d02674 100644 --- a/parser/group_operations.go +++ b/parser/group_operations.go @@ -41,6 +41,7 @@ func containsInt(valid []int, value int) bool { return false } +// addOperationToGroup appends a *types.Operation to an *OperationGroup. func addOperationToGroup( destination *OperationGroup, destinationIndex int, @@ -68,6 +69,11 @@ func addOperationToGroup( } } +// sortOperationGroups returns a slice of OperationGroups sorted by the lowest +// OperationIdentifier.Index in each group. This function also sorts all +// operations in each OperationGroup by OperationIdentifier.Index. It can be +// useful to consumers to have a deterministic ordering of groups and ops within +// each group. func sortOperationGroups(opLen int, opGroups map[int]*OperationGroup) []*OperationGroup { sliceGroups := []*OperationGroup{} @@ -91,13 +97,15 @@ func sortOperationGroups(opLen int, opGroups map[int]*OperationGroup) []*Operati } // GroupOperations parses all of a transaction's opertations and returns a slice -// of each group of related operations. This should ONLY be called on operations -// that have already been asserted for correctness. Assertion ensures there are -// no duplicate operation indexes, operations are sorted, and that operations -// only reference operations with an index less than theirs. +// of each group of related operations (assuming transitive relatedness). This +// should ONLY be called on operations that have already been asserted for +// correctness. Assertion ensures there are no duplicate operation indexes, +// operations are sorted, and that operations only reference operations with +// an index less than theirs. // // OperationGroups are returned in ascending order based on the lowest -// operation index in the group. +// OperationIdentifier.Index in the group. The operations in each OperationGroup +// are also sorted. func GroupOperations(transaction *types.Transaction) []*OperationGroup { ops := transaction.Operations opGroups := map[int]*OperationGroup{} // using a map makes group merges much easier diff --git a/parser/group_operations_test.go b/parser/group_operations_test.go index 4e120dd0..a7652e25 100644 --- a/parser/group_operations_test.go +++ b/parser/group_operations_test.go @@ -33,6 +33,15 @@ func TestSortOperationGroups(t *testing.T) { }, }, }, + 4: { + Operations: []*types.Operation{ + { + OperationIdentifier: &types.OperationIdentifier{ + Index: 4, + }, + }, + }, + }, 0: { Operations: []*types.Operation{ { @@ -45,6 +54,16 @@ func TestSortOperationGroups(t *testing.T) { }, }, }, + { + OperationIdentifier: &types.OperationIdentifier{ + Index: 3, + }, + RelatedOperations: []*types.OperationIdentifier{ + { + Index: 1, + }, + }, + }, { OperationIdentifier: &types.OperationIdentifier{ Index: 0, @@ -52,9 +71,18 @@ func TestSortOperationGroups(t *testing.T) { }, }, }, + 5: { + Operations: []*types.Operation{ + { + OperationIdentifier: &types.OperationIdentifier{ + Index: 5, + }, + }, + }, + }, } - sortedGroups := sortOperationGroups(3, m) + sortedGroups := sortOperationGroups(6, m) assert.Equal(t, []*OperationGroup{ { Operations: []*types.Operation{ @@ -73,6 +101,16 @@ func TestSortOperationGroups(t *testing.T) { }, }, }, + { + OperationIdentifier: &types.OperationIdentifier{ + Index: 3, + }, + RelatedOperations: []*types.OperationIdentifier{ + { + Index: 1, + }, + }, + }, }, }, { @@ -84,6 +122,24 @@ func TestSortOperationGroups(t *testing.T) { }, }, }, + { + Operations: []*types.Operation{ + { + OperationIdentifier: &types.OperationIdentifier{ + Index: 4, + }, + }, + }, + }, + { + Operations: []*types.Operation{ + { + OperationIdentifier: &types.OperationIdentifier{ + Index: 5, + }, + }, + }, + }, }, sortedGroups) } From c3a08dd67920341a3d1639758692d2e207fd2c75 Mon Sep 17 00:00:00 2001 From: Patrick O'Grady Date: Mon, 11 May 2020 21:52:08 -0700 Subject: [PATCH 3/3] Add map comment --- parser/group_operations.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/parser/group_operations.go b/parser/group_operations.go index f5d02674..eeb675f8 100644 --- a/parser/group_operations.go +++ b/parser/group_operations.go @@ -108,7 +108,16 @@ func sortOperationGroups(opLen int, opGroups map[int]*OperationGroup) []*Operati // are also sorted. func GroupOperations(transaction *types.Transaction) []*OperationGroup { ops := transaction.Operations - opGroups := map[int]*OperationGroup{} // using a map makes group merges much easier + + // We use a map of ints to keep track of *OperationGroup instead of a slice + // because merging groups involves removing and combing many items. While we + // could manipulate a slice (leaving holes where groups were merged), it + // seemed less complex to manipulate a map. + // + // Nonetheless, either solution avoids modifying up to `n` opAssignments + // whenever 2 groups merge (this occurs when merging groups in a slice without + // leaving holes). + opGroups := map[int]*OperationGroup{} opAssignments := make([]int, len(ops)) for i, op := range ops { // Create new group