remove icm-services as a go dependency #694
Conversation
iansuvak
requested review from
richardpringle,
geoff-vball,
bernard-avalabs,
michaelkaplan13 and
cam-schultz
| AWM_RELAYER_VERSION=${AWM_RELAYER_VERSION:-'v1.0.0'} | ||
| # ICM_SERVICES_VERSION is needed for the E2E tests but is not a direct dependency since that would create a circular dependency. | ||
| # ICM_SERVICES_VERSION=${ICM_SERVICES_VERSION:-'signature-aggregator-v1.0.0-rc.0'} | ||
| ICM_SERVICES_VERSION=${ICM_SERVICES_VERSION:-'7d1e095dc84913d28c9c82806f240595e8cd4c8b'} |
There was a problem hiding this comment.
Right now this is pointing to a commit on ava-labs/icm-services#616 branch. Should replace when we have the next tag
tests/utils/signature_aggregator.go
Outdated
| SIG_AGG_API_PATH = "/aggregate-signatures" | ||
| ) | ||
|
|
||
| // This is a wrapper around a signature aggregator binar instead of importing the package directly |
There was a problem hiding this comment.
| // This is a wrapper around a signature aggregator binar instead of importing the package directly | |
| // This is a wrapper around a signature aggregator binary instead of importing the package directly |
tests/utils/signature_aggregator.go
Outdated
|
|
||
| const ( | ||
| DEFAULT_SIG_AGG_PATH = "~/.teleporter-deps/icm-services/signature-aggregator" | ||
| DEFAULT_API_PORT = 9090 |
There was a problem hiding this comment.
If it's all the same, let's use port 8080 here, which is the default API port used in icm-services. 9090 is the default metrics port.
There was a problem hiding this comment.
Yup I switched it once when debugging it not starting up before realizing there was a zombie process and never switched it back. Thanks for the catch.
| cmd := exec.CommandContext(ctx, sigAggPath, "--config-file", configFile.Name()) | ||
| cmd.Stdout = os.Stdout | ||
| cmd.Stderr = os.Stderr | ||
| err = cmd.Start() |
There was a problem hiding this comment.
We could start a goroutine here that calls cmd.Wait and emits and error if we get anything unexpected. We do this in the icm-services executable util here.
| cmd.Stdout = os.Stdout | ||
| cmd.Stderr = os.Stderr | ||
| err = cmd.Start() | ||
| Expect(err).Should(BeNil()) |
There was a problem hiding this comment.
We may also want to check the health endpoint before returning, similar to https://github.com/ava-labs/icm-services/blob/25d5c7f6c877067c998b49bdb8b40f1efa2445de/tests/utils/utils.go#L603
tests/utils/signature_aggregator.go
Outdated
| return nil, fmt.Errorf("expected status code 200, got %d", res.StatusCode) | ||
| } | ||
|
|
||
| defer res.Body.Close() |
There was a problem hiding this comment.
Let's move this above the res.StatusCode so that it's called on all non-nil codepaths.
| } | ||
|
|
||
| // Aggregator utils | ||
| func NewSignatureAggregator(apiUri string, l1IDs []ids.ID) *SignatureAggregator { |
There was a problem hiding this comment.
Let's use subnetID instead of l1ID to align with #695
|
|
||
| BUILD_DIR=${ICM_SERVICES_BUILD_PATH}-${ICM_SERVICES_VERSION} | ||
|
|
||
| extract_archive() { |
There was a problem hiding this comment.
[Not a blocker] This function is very similar to the functions of the same name in the Avalanchego and Subnet EVM install scripts. We should combine them. Feel free to defer to a later issue.
| func NewSignatureAggregator(apiUri string, l1IDs []ids.ID) *SignatureAggregator { | ||
| sigAggPath := os.Getenv("SIG_AGG_PATH") | ||
| if sigAggPath == "" { | ||
| sigAggPath = DEFAULT_SIG_AGG_PATH |
There was a problem hiding this comment.
I have a slight preference to treat this as an error rather than falling back to a default path. If for some reason, the env var SIG_AGG_PATH is not set as expected, it seems likely that DEFAULT_SIG_AGG_PATH could already contain a binary with an unknown version, which would be difficult to debug.
I'd prefer that the executable path be passed as an argument, and all OS interfacing be done at the test-suite level rather than in utils, but I understand that would require some additional refactoring. Happy to defer, but could you leave a TODO comment to revist?
Why this should be merged
Removes circular dependency between this repo and icm-services
How this works
Replaces direct calls to signature-aggregator with spinning up an external executable.
How this was tested
E2E tests should pass (this change only affects E2E tests)
How is this documented
TODO