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 <78878559+cam-schultz@users.noreply.github.com> Signed-off-by: Ian Suvak <ian.suvak@avalabs.org>

Co-authored-by: Geoff Stuart <geoff.vball@gmail.com> Signed-off-by: Ian Suvak <ian.suvak@avalabs.org>

…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

 		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

 	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

 		}
+
+		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 <78878559+cam-schultz@users.noreply.github.com> Signed-off-by: Ian Suvak <ian.suvak@avalabs.org>

Update signature-aggregator/README.md

cf689e5

Co-authored-by: Geoff Stuart <geoff.vball@gmail.com> Signed-off-by: Ian Suvak <ian.suvak@avalabs.org>

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 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 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 cam-schultz, geoff-vball and minghinmatthewlam 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 geoff-vball and minghinmatthewlam 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

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

integrate ACP-118 #408

integrate ACP-118 #408

Conversation

feuGeneA commented Aug 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why this should be merged

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

geoff-vball commented Aug 19, 2024

Uh oh!

iansuvak commented Aug 19, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iansuvak Sep 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cam-schultz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

7 participants

feuGeneA commented Aug 2, 2024 •

edited

Loading

iansuvak Sep 5, 2024 •

edited

Loading