Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

integrate ACP-118 #408

Merged
merged 48 commits into from
Sep 6, 2024
Merged

integrate ACP-118 #408

merged 48 commits into from
Sep 6, 2024

Conversation

feuGeneA
Copy link
Contributor

@feuGeneA feuGeneA commented Aug 2, 2024

Why this should be merged

fixes #383

@feuGeneA feuGeneA force-pushed the signature-aggregation-api-acp-118 branch from 40ef7a5 to c46fc41 Compare August 5, 2024 14:14
reqBytes, err = msg.RequestToBytes(codec, req)
}
reqBytes, err := proto.Marshal(
&sdk.SignatureRequest{Message: unsignedMessage.Bytes()},
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed in 557262a

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

last piece addressed in 29cac30

tests/utils/utils.go Outdated Show resolved Hide resolved
reqBytes, err = msg.RequestToBytes(codec, req)
}
reqBytes, err := proto.Marshal(
&sdk.SignatureRequest{Message: unsignedMessage.Bytes()},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

tests/signature_aggregator_api.go Show resolved Hide resolved
Base automatically changed from signature-aggregation-api to main August 8, 2024 20:04
@meaghanfitzgerald meaghanfitzgerald added the Warp Signature API API service for serving arbitrary Warp signatures from any VM label Aug 9, 2024
@geoff-vball
Copy link
Contributor

@feuGeneA is this PR still relevant?

@iansuvak
Copy link
Contributor

@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

@iansuvak iansuvak removed the DO NOT MERGE This PR is not meant to be merged in its current state label Sep 3, 2024
feuGeneA added a commit that referenced this pull request Sep 4, 2024
@feuGeneA feuGeneA force-pushed the signature-aggregation-api-acp-118 branch from 9a0224b to 6cd8245 Compare September 4, 2024 21:34
@@ -43,6 +44,7 @@ func instantiateAggregator(t *testing.T) (
1024,
sigAggMetrics,
messageCreator,
time.Now().Add(-1*time.Minute),
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the purpose of the -1 minute?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

A comment clarifying this would be helpful

Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -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"`

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@iansuvak iansuvak Sep 5, 2024

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
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"),

Choose a reason for hiding this comment

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

we have HexOrCB58ToID and SanitizieHexString utils functions that might apply here

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

fundedAddress,
relayerKey,
)
relayerConfig.EtnaTime = time.Now().AddDate(0, 0, -1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Addressed in 3d7707d

Copy link
Collaborator

@cam-schultz cam-schultz left a comment

Choose a reason for hiding this comment

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

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

tests/utils/utils.go Outdated Show resolved Hide resolved
justification []byte,
sourceSubnet ids.ID,
) ([]byte, error) {
if !s.etnaTime.IsZero() && s.etnaTime.Before(time.Now()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Addressed in 576a375

@feuGeneA feuGeneA merged commit 1bd4dab into main Sep 6, 2024
8 checks passed
@feuGeneA feuGeneA deleted the signature-aggregation-api-acp-118 branch September 6, 2024 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Warp Signature API API service for serving arbitrary Warp signatures from any VM
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Use ACP-118 interface
6 participants