[DRAFT] feat: CLI integration for notification services #299

gpabst · 2025-03-14T11:12:13Z

eigenlayer notifications - Subscribe and unsubscribe to EigenLayer events via the notification service

USAGE:
   eigenlayer notifications command [command options] 

COMMANDS:
   list-events  List all the events available to be subscribed to via notification service for a given AVS
   list-avs     List all the AVSs available to be subscribed to via notification service
   subscribe    Subscribe to an event via the notification service
   unsubscribe  Unsubscribe from an existging subscription via the notification service
   help, h      Shows a list of commands or help for one command

OPTIONS:
   --help, -h  show help

Samples with stub data generated by AI:

eigenlayer notifications list-events --avs-name eigenda
Available Events for AVS 'eigenda':
----------------
Event Name: DataStored
Contract Address: 0x6d50c205ca13f2b89d1021efd9eed37d10ea8bef
Ethereum Topic: 0x9c72852172521097ba7e1482e6b44b351323df0155f97f3e2772a0c3b17044a9
----------------
Event Name: DataRetrieved
Contract Address: 0x6d50c205ca13f2b89d1021efd9eed37d10ea8bef
Ethereum Topic: 0x8c0f34a9a45f07d3c8a7c56f95009c407c44f78f35ee7dba849edd9a08e224e6

eigenlayer notifications list-avs                            
Available AVS Services:
----------------------
Name: eigenda
Display Name: EigenDA
Description: Decentralized data availability layer for rollups
Status: active
----------------------
Name: eigenlayer-operator
Display Name: EigenLayer Operator
Description: Core EigenLayer operator service
Status: active
----------------------
Name: eigenlayer-node
Display Name: EigenLayer Node
Description: EigenLayer node service for validators
Status: active
----------------------
Name: restaking-service
Display Name: EigenLayer Restaking
Description: Restaking service for ETH and LSTs
Status: active
----------------------
Name: avs-marketplace
Display Name: AVS Marketplace
Description: Marketplace for AVS services and operators
Status: planned

./eigenlayer notifications subscribe --avs-name eigenda --event-name DataStored --operator-id "operator12345" --delivery-method "email" --delivery-details "[email protected]"

Successfully subscribed to DataStored events for AVS 'eigenda'

nelsonijih · 2025-03-14T14:06:58Z

would be great to see sample out of the commands.

bdchatham · 2025-03-14T14:51:32Z

pkg/notifications/flags.go

+var (
+	AvsNameFlag = cli.StringFlag{
+		Name:     "avs-name",
+		Aliases:  []string{"k"},


Alias names seem off on a few of these flags.

bdchatham · 2025-03-14T14:52:06Z

pkg/notifications.go

+	"github.com/urfave/cli/v2"
+)
+
+func NotificationsCmd(p utils.Prompter) *cli.Command {


Can we use the BaseCommand pattern for new commands?

bdchatham · 2025-03-14T14:52:56Z

cmd/eigenlayer/main.go

@@ -39,6 +39,7 @@ func main() {
 		return nil
 	}

+	app.Commands = append(app.Commands, pkg.NotificationsCmd(prompter))


nit: idiomatic go is New.. for factory methods.

bdchatham · 2025-03-14T14:54:48Z

pkg/notifications/list_events.go

+	Message    string `json:"message"`
+}
+
+const baseURL = "http://localhost:3000/api"


Would suggest passing this as a variable for better testing and parameterization.

bdchatham · 2025-03-14T14:55:57Z

pkg/notifications/list_events.go

+type ErrorResponse struct {
+	StatusCode int    `json:"statusCode"`
+	Message    string `json:"message"`
+}


Can we move these to a shared location? It's going to become a common theme.

bdchatham · 2025-03-14T15:06:23Z

pkg/notifications/subscribe.go

+var DeliveryMethodFlag = cli.StringFlag{
+	Name:     "delivery-method",
+	Usage:    "Method of delivery for notifications (email, webhook, or telegram)",
+	Required: true,
+}
+
+var DeliveryDetailsFlag = cli.StringFlag{
+	Name:     "delivery-details",
+	Usage:    "Details for the delivery method (email address, webhook URL, or telegram chat ID)",
+	Required: true,
+}


Any reason to not have this in the flag file?

bdchatham · 2025-03-14T15:12:01Z

pkg/notifications/subscribe.go

+}
+
+func validateDeliveryMethod(method string) error {
+	validMethods := map[string]bool{


nit: every time you call this method you create a new map that never changes. You can turn this into a var. Once you do this you don't really need the method unless you are trying to standardize the error message. Any caller who uses the method anyways similarly has to have an if err { ... } clause.

bdchatham · 2025-03-14T15:15:10Z

pkg/notifications/subscribe.go

+			}
+
+			// Accept both 200 and 201 as success codes
+			if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusCreated {


What is the difference from your service perspective in these two cases?

bdchatham · 2025-03-14T15:24:22Z

pkg/notifications/subscribe.go

+	return nil
+}
+
+func SubscribeEventsCmd() *cli.Command {


Similar to the other long method, this should be broken down. There are very different, separated tasks here between input validation & parsing, http request dispatch & processing and response handling.

I think it's important for us trend away from monolithic commands and not towards them.

bdchatham · 2025-03-14T15:27:31Z

pkg/notifications/list_events.go

+
+const baseURL = "http://localhost:3000/api"
+
+func ListEventsCmd() *cli.Command {


This command is a single function, 60 lines long. I think it's worth discussing why there seems to be a preference for large, monolithic commands, rather than following the principle of code segments doing one thing, and one thing clearly.

MadelineAu · 2025-03-17T00:13:05Z

pkg/notifications.go

+func NotificationsCmd(p utils.Prompter) *cli.Command {
+	var notificationsCmd = &cli.Command{
+		Name:  "notifications",
+		Usage: "Subscribe and unsubscribe to EigenLayer events via the notification service",


Notification Service?

MadelineAu · 2025-03-17T00:14:39Z

pkg/notifications/list_avs.go

+		Usage:     "List all the AVSs available to be subscribed to via notification service",
+		UsageText: "list-avs",
+		Description: `
+This command provides a listing of all AVS services with events available to be subscribed via the notification service.


This command provides -> Provides

'This command' is implied by this being CLI help

gpabst · 2025-03-17T10:22:12Z

Thanks for the feedback on the draft, will work through it. I have pushed some refactoring already that should resolve some of the code issues.

bdchatham · 2025-03-17T14:57:17Z

pkg/notifications/unsubscribe.go

+	if !isSuccessStatusCode(resp.StatusCode) {
+		return handleErrorResponse(resp.StatusCode, body)
+	}


This can be moved into the HTTP code path, no? Same for the other place it's called.

bdchatham · 2025-03-17T14:58:27Z

pkg/notifications/utils.go

+	return APIBaseURL
+}
+
+// doHTTPRequest performs the HTTP request and processes the response


Is this really utils or is it an http client? Seems like something we want to generalize and use in other places, like ListOperatorApplications for RMS.

bdchatham · 2025-03-17T14:59:33Z

pkg/notifications/utils.go

+
+// getHTTPClient returns the shared HTTP client instance
+func getHTTPClient() *http.Client {
+	httpClientOnce.Do(func() {


If you turn this into a normal object you don't need to enforce only-once instantiation.

bdchatham · 2025-03-17T15:00:29Z

pkg/notifications/utils.go

+)
+
+// defaultTimeout is the default timeout for HTTP requests
+const defaultTimeout = 10 * time.Second


It's good to have a default, but this should also be parameterized for overriding and testing.

bdchatham · 2025-03-17T15:01:46Z

pkg/notifications/utils.go

+	return makeRequest(ctx, config)
+}
+
+// makeDeleteRequest makes a DELETE request to the specified URL with proper headers


nit: is it meaningful to have comments like this?

bdchatham · 2025-03-17T15:02:45Z

pkg/notifications/types.go

+}
+
+// AvailableAvsResponseDto represents the API response for available AVS services.
+type AvailableAvsResponseDto struct {


Service question: how does available AVS list get populated server side?

bdchatham · 2025-03-17T15:05:56Z

pkg/notifications/flags.go

+	SubscriptionIDFlag = cli.StringFlag{
+		Name:     "subscription-id",
+		Aliases:  []string{"id"},
+		Required: false,


Based on your validation, this is required.

bdchatham · 2025-03-17T15:06:02Z

pkg/notifications/flags.go

+	AvsNameFlag = cli.StringFlag{
+		Name:     "avs-name",
+		Aliases:  []string{"avs"},
+		Required: false,


Based on your validation, this is required.

bdchatham · 2025-03-17T15:06:51Z

pkg/notifications/subscribe.go

+func validateSubscribeParams(cliCtx *cli.Context) (*SubscriptionParams, error) {
+	params := &SubscriptionParams{}
+
+	params.AvsName = cliCtx.String("avs-name")
+	if params.AvsName == "" {
+		return nil, fmt.Errorf("avs-name is required")
+	}
+
+	params.EventName = cliCtx.String("event-name")
+	if params.EventName == "" {
+		return nil, fmt.Errorf("event-name is required")
+	}
+
+	params.OperatorID = cliCtx.String("operator-id")
+	if params.OperatorID == "" {
+		return nil, fmt.Errorf("operator-id is required")
+	}
+
+	params.DeliveryMethod = cliCtx.String("delivery-method")
+	if err := validateDeliveryMethod(params.DeliveryMethod); err != nil {
+		return nil, err
+	}
+
+	params.DeliveryDetails = cliCtx.String("delivery-details")
+	if params.DeliveryDetails == "" {
+		return nil, fmt.Errorf("delivery-details is required")
+	}
+
+	return params, nil
+}


Not clear to me why we need this if you are marking these flags as required with urfacecli directly. It will automatically validate them, right?

feat: CLI integration for notification services

48acef3

nelsonijih marked this pull request as ready for review March 14, 2025 14:05

gpabst changed the title ~~feat: CLI integration for notification services~~ [DRAFT]feat: CLI integration for notification services Mar 14, 2025

gpabst changed the title ~~[DRAFT]feat: CLI integration for notification services~~ [DRAFT] feat: CLI integration for notification services Mar 14, 2025

gpabst marked this pull request as draft March 14, 2025 14:12

bdchatham reviewed Mar 14, 2025

View reviewed changes

refactor: use new more RESTful API

e941c36

MadelineAu reviewed Mar 17, 2025

View reviewed changes

gpabst added 2 commits March 17, 2025 09:05

chore: make format

d2a5907

fix: lint

c2e78eb

bdchatham reviewed Mar 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DRAFT] feat: CLI integration for notification services #299

[DRAFT] feat: CLI integration for notification services #299

gpabst commented Mar 14, 2025 •

edited

Loading

nelsonijih commented Mar 14, 2025

bdchatham Mar 14, 2025

bdchatham Mar 14, 2025

bdchatham Mar 14, 2025

bdchatham Mar 14, 2025

bdchatham Mar 14, 2025

bdchatham Mar 14, 2025

bdchatham Mar 14, 2025

bdchatham Mar 14, 2025

bdchatham Mar 14, 2025

bdchatham Mar 14, 2025

MadelineAu Mar 17, 2025

MadelineAu Mar 17, 2025

gpabst commented Mar 17, 2025

bdchatham Mar 17, 2025

bdchatham Mar 17, 2025

bdchatham Mar 17, 2025

bdchatham Mar 17, 2025 •

edited

Loading

bdchatham Mar 17, 2025

bdchatham Mar 17, 2025 •

edited

Loading

bdchatham Mar 17, 2025

bdchatham Mar 17, 2025

bdchatham Mar 17, 2025


		const baseURL = "http://localhost:3000/api"

		func ListEventsCmd() *cli.Command {

[DRAFT] feat: CLI integration for notification services #299

Are you sure you want to change the base?

[DRAFT] feat: CLI integration for notification services #299

Conversation

gpabst commented Mar 14, 2025 • edited Loading

nelsonijih commented Mar 14, 2025

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

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

gpabst commented Mar 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdchatham Mar 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdchatham Mar 17, 2025 • 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

gpabst commented Mar 14, 2025 •

edited

Loading

bdchatham Mar 17, 2025 •

edited

Loading

bdchatham Mar 17, 2025 •

edited

Loading