Signature Aggregator: Happy path unit test #467

feuGeneA · 2024-08-30T14:48:40Z

addresses review comment #457 (comment)

Why this should be merged

How this works

How this was tested

How is this documented

addresses review comment #457 (comment)

geoff-vball · 2024-09-05T13:50:20Z

signature-aggregator/aggregator/aggregator_test.go

 func TestCreateSignedMessageFailsWithNoValidators(t *testing.T) {
 	aggregator, mockNetwork := instantiateAggregator(t)
 	msg, err := warp.NewUnsignedMessage(0, ids.Empty, []byte{})
-	require.Equal(t, err, nil)
+	require.Equal(t, nil, err)


We should use require.NoError(t, err) here and elsewhere

addressed in 0c630f0

cam-schultz · 2024-09-05T14:33:42Z

signature-aggregator/aggregator/aggregator_test.go

+
+func TestCreateSignedMessageSucceeds(t *testing.T) {
+	var msg *warp.UnsignedMessage // to be signed
+	chainID, err := ids.ToID(utils.RandomBytes(32))


Let's use ids.GenerateTestID here.

addressed in 408b68f

cam-schultz · 2024-09-05T14:34:40Z

signature-aggregator/aggregator/aggregator_test.go

+func TestCreateSignedMessageSucceeds(t *testing.T) {
+	var msg *warp.UnsignedMessage // to be signed
+	chainID, err := ids.ToID(utils.RandomBytes(32))
+	if err != nil {


Let's use require.NoError rather than panicking.

this isn't necessary now that we're using GenerateTestID, since it doesn't return an error

cam-schultz · 2024-09-05T14:36:08Z

signature-aggregator/aggregator/aggregator_test.go

+	if err != nil {
+		panic(err)
+	}
+	networkID := uint32(0)


Let's use constants.UnitTestID

addressed in fc24e7a

cam-schultz · 2024-09-05T14:37:09Z

signature-aggregator/aggregator/aggregator_test.go

+	aggregator, mockNetwork := instantiateAggregator(t)
+
+	subnetID, err := ids.ToID(utils.RandomBytes(32))
+	require.Equal(t, nil, err)


Suggested change

require.Equal(t, nil, err)

require.NoError(t, err)

addressed in 0c630f0

cam-schultz · 2024-09-05T14:37:21Z

signature-aggregator/aggregator/aggregator_test.go

+
+	aggregator, mockNetwork := instantiateAggregator(t)
+
+	subnetID, err := ids.ToID(utils.RandomBytes(32))


Let's use ids.GenerateTestID

addressed in 408b68f

addresses review comment #467 (comment)

geoff-vball · 2024-09-05T19:48:52Z

signature-aggregator/aggregator/aggregator_test.go

 	networkID := uint32(0)
-	msg, err = warp.NewUnsignedMessage(
+	msg, err := warp.NewUnsignedMessage(


With this change, we don't need the declaration for msg on line 237

Yes but I like the readability of having the declaration there. The name of the test/function tells you a message is going to be signed, and then the very first line shows you (not just tells you with comments) that we're starting to construct the message to be signed.

Order mock validator set

…py-path

iansuvak · 2024-09-10T14:42:40Z

peers/app_request_network.go

@@ -212,12 +212,12 @@ type ConnectedCanonicalValidators struct {
 	ConnectedWeight       uint64
 	TotalValidatorWeight  uint64
 	ValidatorSet          []*warp.Validator
-	nodeValidatorIndexMap map[ids.NodeID]int
+	NodeValidatorIndexMap map[ids.NodeID]int


I'm not sure that I love this struct having all of it's fields exported instead of using getters/setters but every other field is already exported so not a blocker for this PR

iansuvak · 2024-09-10T14:54:19Z

signature-aggregator/aggregator/cache/cache.go

@@ -41,7 +42,16 @@ func (c *Cache) Get(msgID ids.ID) (map[PublicKeyBytes]SignatureBytes, bool) {
 	cachedValue, isCached := c.signatures.Get(msgID)

 	if isCached {
-		c.logger.Debug("cache hit", zap.Stringer("msgID", msgID))
+		var encodedKeys []string
+		for key := range cachedValue {


This isn't a very performance sensitive applications but I think it's weird that we are doing this work here just to construct the Debug level log message.

In practice this ends up being O(n^2) cost for debug logging

Great point, thanks for calling it out. I removed the public key logging in fd04dc5

Yeah if it was useful we could have gotten around by checking log level or putting it in a function call if this logger supports lazy eval.

Not saying it's worth while here but it looks like:
if s.logger.Enabled(logging.Debug) {
would let us do more complex logging when debugging.

addresses review comment https://github.com/ava-labs/awm-relayer/pull/467/files#r1752146075

feuGeneA force-pushed the sig-agg-unit-test-happy-path branch 3 times, most recently from a7c907e to e073828 Compare August 30, 2024 19:06

Base automatically changed from sig-agg-unit-tests to main August 30, 2024 19:06

feuGeneA changed the title ~~happy path test~~ Signature Aggregator: Happy path unit test Aug 30, 2024

feuGeneA force-pushed the sig-agg-unit-test-happy-path branch from e073828 to 15aac03 Compare August 30, 2024 19:07

happy path test

23a4a41

addresses review comment #457 (comment)

feuGeneA force-pushed the sig-agg-unit-test-happy-path branch from 15aac03 to 23a4a41 Compare September 5, 2024 02:05

feuGeneA marked this pull request as ready for review September 5, 2024 03:04

feuGeneA requested a review from a team as a code owner September 5, 2024 03:04

feuGeneA requested review from iansuvak, geoff-vball, minghinmatthewlam, bernard-avalabs, michaelkaplan13 and cam-schultz September 5, 2024 03:04

geoff-vball reviewed Sep 5, 2024

View reviewed changes

cam-schultz reviewed Sep 5, 2024

View reviewed changes

feuGeneA added 3 commits September 5, 2024 19:25

use require.NoError

0c630f0

addresses review comment #467 (comment)

use GenerateTestID and (GenerateTestNodeID)

408b68f

addresses review comment #467 (comment)

use constants.UnitTestID

fc24e7a

feuGeneA requested review from geoff-vball and cam-schultz September 5, 2024 19:47

geoff-vball reviewed Sep 5, 2024

View reviewed changes

feuGeneA and others added 2 commits September 6, 2024 08:02

Merge branch 'main' into sig-agg-unit-test-happy-path

72552b0

Order mock validator set

6b839ed

michaelkaplan13 mentioned this pull request Sep 6, 2024

Order mock validator set #481

Merged

michaelkaplan13 and others added 3 commits September 6, 2024 19:01

Unexport struct

142df89

Merge pull request #481 from ava-labs/order-mock-validator-set

170e72b

Order mock validator set

Merge remote-tracking branch 'origin/main' into sig-agg-unit-test-hap…

30b301e

…py-path

geoff-vball approved these changes Sep 10, 2024

View reviewed changes

Merge branch 'main' into sig-agg-unit-test-happy-path

8771a32

iansuvak reviewed Sep 10, 2024

View reviewed changes

stop logging public keys on signature cache hit

fd04dc5

addresses review comment https://github.com/ava-labs/awm-relayer/pull/467/files#r1752146075

feuGeneA requested a review from iansuvak September 10, 2024 16:50

iansuvak approved these changes Sep 10, 2024

View reviewed changes

geoff-vball and others added 2 commits September 10, 2024 13:23

Merge branch 'main' into sig-agg-unit-test-happy-path

75927b1

adapt to ids.RequestID changes

2a475ea

feuGeneA requested review from iansuvak and geoff-vball September 10, 2024 21:27

geoff-vball approved these changes Sep 10, 2024

View reviewed changes

iansuvak approved these changes Sep 10, 2024

View reviewed changes

feuGeneA merged commit 3f911e9 into main Sep 11, 2024
8 checks passed

feuGeneA deleted the sig-agg-unit-test-happy-path branch September 11, 2024 23:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Signature Aggregator: Happy path unit test #467

Signature Aggregator: Happy path unit test #467

feuGeneA commented Aug 30, 2024

geoff-vball Sep 5, 2024

feuGeneA Sep 5, 2024

cam-schultz Sep 5, 2024

feuGeneA Sep 5, 2024

cam-schultz Sep 5, 2024

feuGeneA Sep 5, 2024

cam-schultz Sep 5, 2024

feuGeneA Sep 5, 2024

cam-schultz Sep 5, 2024

feuGeneA Sep 5, 2024

cam-schultz Sep 5, 2024

feuGeneA Sep 5, 2024

geoff-vball Sep 5, 2024

feuGeneA Sep 6, 2024

iansuvak Sep 10, 2024

iansuvak Sep 10, 2024

feuGeneA Sep 10, 2024

iansuvak Sep 10, 2024

iansuvak Sep 11, 2024


		aggregator, mockNetwork := instantiateAggregator(t)

		subnetID, err := ids.ToID(utils.RandomBytes(32))

Signature Aggregator: Happy path unit test #467

Signature Aggregator: Happy path unit test #467

Conversation

feuGeneA commented Aug 30, 2024

Why this should be merged

How this works

How this was tested

How is this documented

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

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