Skip to content

Conversation

esuwu
Copy link
Contributor

@esuwu esuwu commented Mar 5, 2024

No description provided.

func (e DataEntries) String() string {
var resStr string
for _, entry := range e {
entryStr := entry.GetKey() + entry.GetValueType().String()
Copy link
Contributor

Choose a reason for hiding this comment

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

No divider between key and value?

}

// SortDataEntries sorts DataEntries slice by entry keys.
func SortDataEntries(entries []DataEntry) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this function can be package private. Or, it can be declared as a method of DataEntries slice

}

func (e DataEntries) String() string {
var resStr string
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to use stings.Builder for concatenating many strings.

return true, nil
}

func SortAtomicSnapshotsByType(snapshots TxSnapshot) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this function can be package private. Or, it can be declared as a method of TxSnapshot slice. And, is it really necessary to create function when the type implements sort.Interface?

return entry, nil
}

func CompareDataEntry(a, b DataEntry) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary to create such function? i think you can use the same approach as for AtomicSnapshots. Create Equal method for each DataEntry type and use the new method for comparison.

addr2, _ := proto.NewAddressFromString("3P9o3uwx3fWZz3b5aaaaaaaaaaFoPW6z7HB")
assetID1 := crypto.MustDigestFromBase58("BrjV5AB5S7qN5tLQFbU5tpLj5qeozfVvPxEpDkmmhNP")
assetID2 := crypto.MustDigestFromBase58("5Zv8JLH8TTvq9iCo6HtB2K7CGpTJt6JTj5yvXaDVrxEJ")
publicKey1, _ := crypto.NewPublicKeyFromBase58("5TBjL2VdL1XmXq5dC4SYMeH5sVCGmMTeBNNYqWCuEXMn")
Copy link
Contributor

Choose a reason for hiding this comment

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

And here

Comment on lines +520 to +525
if err != nil {
t.Errorf("Error comparing snapshots: %v", err)
}
if equal != tt.wantEqual {
t.Errorf("Expected snapshots to be equal: %v, got: %v", tt.wantEqual, equal)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use testify package.

}

// Equal compares two DataEntries slices and returns true if they are equal.
func (e DataEntries) Equal(other DataEntries) (bool, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's necessary to say that this function mutates passed argument, i.e. elements order in the slice.


func (s DataEntriesSnapshot) Apply(a SnapshotApplier) error { return a.ApplyDataEntries(s) }

func (s DataEntriesSnapshot) Equal(otherSnapshot AtomicSnapshot) (bool, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's necessary to say that this function mutates passed argument, i.e. elements order in theDataEntries slice.

}

// Equal function assumes that TransactionsSnapshots are in same order in both original and other instances.
func (bs BlockSnapshot) Equal(other BlockSnapshot) (bool, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's necessary to say that this function can mutate the passed argument

@nickeskov nickeskov added do not merge The PR is not ready to be merged temporary It's necessary at the moment, but it's going to be deleted/changed in the nearest future labels Apr 23, 2024
@nickeskov nickeskov marked this pull request as draft April 23, 2024 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge The PR is not ready to be merged temporary It's necessary at the moment, but it's going to be deleted/changed in the nearest future
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants