-
Notifications
You must be signed in to change notification settings - Fork 19
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
Warp API support #345
Warp API support #345
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two small nits, but LGTM. Thanks for putting this together so quickly.
Totally fine with punting on the question in the E2E regarding checking that the API is actually used for now; worth thinking about though IMO
) | ||
// Enable the Warp API for all source blockchains | ||
for _, subnet := range relayerConfig.SourceBlockchains { | ||
subnet.WarpAPIEndpoint = subnet.RPCEndpoint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any (easy) way to check that the API endpoint is actually used by this test? Not necessary right now, but if the configuration changed in the future this test may silently pass if it falls back to using the P2P requests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm that's a good question that I don't immediately know the best answer to. We could add distinct metrics and monitor those from the test. Those metrics would be useful in a deployment as well. I'll look into this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added those metrics in and now check them in the test.
networkLogLevel := logging.Error | ||
if logLevel <= logging.Debug { | ||
networkLogLevel = logLevel | ||
} | ||
network, err := peers.NewNetwork( | ||
networkLogLevel, | ||
registerer, | ||
prometheus.DefaultRegisterer, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While adding in the new get signature metrics, I noticed that we also collect a TON of unrelated network and app requests metrics. This disables these extra metrics from being collected.
Co-authored-by: Michael Kaplan <[email protected]> Signed-off-by: cam-schultz <[email protected]>
Co-authored-by: Michael Kaplan <[email protected]> Signed-off-by: cam-schultz <[email protected]>
Co-authored-by: Michael Kaplan <[email protected]> Signed-off-by: cam-schultz <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙏
Why this should be merged
Fixe #119. Exposes config options to fetch Warp message aggregate signatures via the Warp API rather than app request.
How this works
Adds
SourceBlockchain.WarpAPIEndpoint
config option. Internally,ApplicationRelayers
are composed of[rpc.Client](https://github.com/ava-labs/subnet-evm/blob/master/rpc/client.go)
rather than[warp.Client](https://github.com/ava-labs/subnet-evm/blob/master/warp/client.go)
. This is becausewarp.Client
does not presently support query params or http headers, and expects the URI to be in a specific form.How this was tested
Added Warp API e2e test.
How is this documented
Updated README