integrate ACP-118 #408

feuGeneA · 2024-08-02T14:46:04Z

Why this should be merged

fixes #383

signature-aggregator/aggregator/aggregator.go

…pi-acp-118

iansuvak · 2024-08-05T21:51:36Z

signature-aggregator/aggregator/aggregator.go

-		reqBytes, err = msg.RequestToBytes(codec, req)
-	}
+	reqBytes, err := proto.Marshal(
+		&sdk.SignatureRequest{Message: unsignedMessage.Bytes()},


We should add justification bytes here as well as the API. In the API they should be hex encoded and since their meaning is VM specific we don't need to do any processing to them either just like with the message. We should only pass the non-zero fields since the ACP-118 spec but in the API layer we should do validation that at least one of message, justification fields is non-zero.

I would also rename our API to use Message instead of UnsignedMessage to match the ACP-118 spec while we are at it

addressed in 557262a

This isn't done yet. I added the justification everywhere (in the API) except for right here 😅

last piece addressed in 29cac30

addresses review comment #408 (comment)

…signature-aggregation-api-acp-118

tests/utils/utils.go

feuGeneA · 2024-08-07T16:13:36Z

signature-aggregator/aggregator/aggregator.go

-		reqBytes, err = msg.RequestToBytes(codec, req)
-	}
+	reqBytes, err := proto.Marshal(
+		&sdk.SignatureRequest{Message: unsignedMessage.Bytes()},


This isn't done yet. I added the justification everywhere (in the API) except for right here 😅

tests/signature_aggregator_api.go

Co-authored-by: cam-schultz <[email protected]> Signed-off-by: Ian Suvak <[email protected]>

Co-authored-by: Geoff Stuart <[email protected]> Signed-off-by: Ian Suvak <[email protected]>

…acp-118

geoff-vball · 2024-08-19T12:38:56Z

@feuGeneA is this PR still relevant?

iansuvak · 2024-08-19T13:25:35Z

@geoff-vball

@feuGeneA is this PR still relevant?

It hasn't been merged yet so yes it is. I will update my draft PR (#422) that goes into this PR and then we can bring this one up to speed with main and then it should be ready for review

…ersions.sh" This reverts commit 47cfef6.

Add `etna-time` configs

addresses review comment #408 (comment)

geoff-vball · 2024-09-04T21:59:37Z

signature-aggregator/aggregator/aggregator_test.go

@@ -43,6 +44,7 @@ func instantiateAggregator(t *testing.T) (
 		1024,
 		sigAggMetrics,
 		messageCreator,
+		time.Now().Add(-1*time.Minute),


What's the purpose of the -1 minute?

This is the etnaTime parameter. This would make the aggregator in this test run using the post-etna code paths since in this case Etna was activated a minute ago

A comment clarifying this would be helpful

tests/etna_upgrade.go

minghinmatthewlam · 2024-09-05T17:15:34Z

relayer/config/config.go

@@ -67,6 +68,9 @@ type Config struct {
 	DeciderURL             string                   `mapstructure:"decider-url" json:"decider-url"`
 	SignatureCacheSize     uint64                   `mapstructure:"signature-cache-size" json:"signature-cache-size"`

+	// mapstructure doesn't handle time.Time out of the box so handle it manually
+	EtnaTime time.Time `json:"etna-time"`


we could take in a string as well and use time.Parse in a date foramt

I like that the v.GetTime() call will handle both RFC3339 format as well as UNIX timestamps cleanly and output it unambiguously when being written out.

With time.Parse we would need to manually handle conversion when writing it out to disk and reading it separately.

signature-aggregator/aggregator/aggregator.go

minghinmatthewlam · 2024-09-05T17:23:28Z

signature-aggregator/api/api.go

 		if err != nil {
 			msg := "Error unpacking warp message"
 			logger.Warn(msg, zap.Error(err))
 			writeJSONError(logger, w, msg)
 			return
 		}
+
+		justification, err := hex.DecodeString(
+			strings.TrimPrefix(req.Justification, "0x"),


we have HexOrCB58ToID and SanitizieHexString utils functions that might apply here

This doesn't decode to an ID but yeah we should call SanitizeHexString here

cam-schultz · 2024-09-05T17:46:44Z

tests/etna_upgrade.go

+		fundedAddress,
+		relayerKey,
+	)
+	relayerConfig.EtnaTime = time.Now().AddDate(0, 0, -1)


Same here, a comment clarifying the -1 would be helpful

Addressed in 3d7707d

…-api-acp-118

cam-schultz

A handful of final nits, but once they as well as the other open comments are addressed, this LGTM

tests/utils/utils.go

cam-schultz · 2024-09-05T17:50:45Z

signature-aggregator/aggregator/aggregator.go

+	justification []byte,
+	sourceSubnet ids.ID,
+) ([]byte, error) {
+	if !s.etnaTime.IsZero() && s.etnaTime.Before(time.Now()) {


It would be nice to combine these two conditions into a helper s.EtnaActivated()

Addressed in 576a375

iansuvak reviewed Aug 2, 2024

View reviewed changes

signature-aggregator/aggregator/aggregator.go Outdated Show resolved Hide resolved

go.mod: use latest subnet-evm commit

c46fc41

feuGeneA force-pushed the signature-aggregation-api-acp-118 branch from 40ef7a5 to c46fc41 Compare August 5, 2024 14:14

feuGeneA added 3 commits August 5, 2024 14:27

integrate w/ACP-118 protobuf SignatureRequest

650c987

test relayer & aggregator: log debug by default

a9c7768

Merge branch 'signature-aggregation-api' into signature-aggregation-a…

9db9153

…pi-acp-118

iansuvak reviewed Aug 5, 2024

View reviewed changes

feuGeneA added 6 commits August 6, 2024 15:31

add justification, rename message to drop Unsigned

557262a

addresses review comment #408 (comment)

Merge remote-tracking branch 'origin/signature-aggregation-api' into …

b56e5d3

…signature-aggregation-api-acp-118

Merge remote-tracking branch 'origin/signature-aggregation-api' into …

adb8fb2

…signature-aggregation-api-acp-118

re-pin dependencies

f399cff

Merge remote-tracking branch 'origin/signature-aggregation-api' into …

fa3dbc0

…signature-aggregation-api-acp-118

sync teleporter git submodule to go.mod version

c771ca9

feuGeneA commented Aug 7, 2024

View reviewed changes

Base automatically changed from signature-aggregation-api to main August 8, 2024 20:04

iansuvak and others added 11 commits August 8, 2024 22:41

version bumps, logging and handlerid packing hacks

fa60e03

Update signature-aggregator/README.md

0108081

Co-authored-by: cam-schultz <[email protected]> Signed-off-by: Ian Suvak <[email protected]>

Update signature-aggregator/README.md

cf689e5

Co-authored-by: Geoff Stuart <[email protected]> Signed-off-by: Ian Suvak <[email protected]>

address review feedback

b186d1c

invert utils.CheckStakeWeightPercentageExceedsThreshold

95797af

Fix bugs in api and remove old testing code

b8735b0

bump teleporter submodule

0be46eb

Merge remote-tracking branch 'origin/main' into isuvak/sig-agg-acp-118

e3cefff

actually fix versions

79132a1

Merge branch 'main' into signature-aggregation-api-acp-118

b5cff9e

Merge branch 'signature-aggregation-api-acp-118' into isuvak/sig-agg-…

2f337c9

…acp-118

meaghanfitzgerald added the Warp Signature API API service for serving arbitrary Warp signatures from any VM label Aug 9, 2024

replace feature branch commit references with default branch ones

bce634d

iansuvak added 4 commits August 29, 2024 10:02

Revert "use extract_commit instead of manually synced references in v…

b87bcc1

…ersions.sh" This reverts commit 47cfef6.

Merge remote-tracking branch 'origin/main' into etna-time

5852f6c

Add TODO to cleanup after Etna release

7f9488c

Merge pull request #463 from ava-labs/etna-time

0648123

Add `etna-time` configs

iansuvak removed the DO NOT MERGE This PR is not meant to be merged in its current state label Sep 3, 2024

iansuvak mentioned this pull request Sep 3, 2024

Add feature flags for network protocol level changes. #440

Closed

iansuvak and others added 3 commits September 4, 2024 10:40

Merge branch 'main' into signature-aggregation-api-acp-118

ae43d72

adapt to latest changes in main

ca06934

pass nil, not empty slice

6cd8245

addresses review comment #408 (comment)

feuGeneA added a commit that referenced this pull request Sep 4, 2024

versions.sh: pull versions from go.mod again

9a0224b

addresses review comment #408 (comment)

feuGeneA force-pushed the signature-aggregation-api-acp-118 branch from 9a0224b to 6cd8245 Compare September 4, 2024 21:34

Merge branch 'main' into signature-aggregation-api-acp-118

b04df21

feuGeneA requested review from geoff-vball, minghinmatthewlam and cam-schultz September 4, 2024 21:37

geoff-vball reviewed Sep 4, 2024

View reviewed changes

tests/etna_upgrade.go Show resolved Hide resolved

minghinmatthewlam reviewed Sep 5, 2024

View reviewed changes

cam-schultz reviewed Sep 5, 2024

View reviewed changes

iansuvak added 2 commits September 5, 2024 14:26

address review feedback

3d7707d

Merge remote-tracking branch 'origin/main' into signature-aggregation…

2ba7b61

…-api-acp-118

cam-schultz approved these changes Sep 5, 2024

View reviewed changes

address review feedback

576a375

cam-schultz approved these changes Sep 5, 2024

View reviewed changes

iansuvak requested review from minghinmatthewlam and geoff-vball September 5, 2024 19:54

geoff-vball approved these changes Sep 5, 2024

View reviewed changes

minghinmatthewlam approved these changes Sep 5, 2024

View reviewed changes

feuGeneA merged commit 1bd4dab into main Sep 6, 2024
8 checks passed

feuGeneA deleted the signature-aggregation-api-acp-118 branch September 6, 2024 12:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

integrate ACP-118 #408

integrate ACP-118 #408

feuGeneA commented Aug 2, 2024 •

edited

Loading

iansuvak Aug 5, 2024

feuGeneA Aug 6, 2024

feuGeneA Aug 7, 2024

feuGeneA Aug 21, 2024

feuGeneA Aug 7, 2024

geoff-vball commented Aug 19, 2024

iansuvak commented Aug 19, 2024

geoff-vball Sep 4, 2024

iansuvak Sep 4, 2024

cam-schultz Sep 5, 2024

iansuvak Sep 5, 2024

minghinmatthewlam Sep 5, 2024

iansuvak Sep 5, 2024 •

edited

Loading

minghinmatthewlam Sep 5, 2024

iansuvak Sep 5, 2024

iansuvak Sep 5, 2024

cam-schultz Sep 5, 2024

iansuvak Sep 5, 2024

cam-schultz left a comment

cam-schultz Sep 5, 2024

iansuvak Sep 5, 2024

integrate ACP-118 #408

integrate ACP-118 #408

Conversation

feuGeneA commented Aug 2, 2024 • edited Loading

Why this should be merged

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

geoff-vball commented Aug 19, 2024

iansuvak commented Aug 19, 2024

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

iansuvak Sep 5, 2024 • edited Loading

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

cam-schultz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feuGeneA commented Aug 2, 2024 •

edited

Loading

iansuvak Sep 5, 2024 •

edited

Loading