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

feat(cmd): edit finality provider description #90

Merged
merged 19 commits into from
Oct 15, 2024
Merged

Conversation

Lazar955
Copy link
Member

@Lazar955 Lazar955 commented Oct 10, 2024

  • Adds CLI support to edit finality provider description

Note: I did try using the edit cmd defined in Babylon, but I wasn't successful as the cmd context is getting passed into the edit cmd resulting in a flags error.

References issue

@Lazar955 Lazar955 marked this pull request as draft October 10, 2024 14:27
@Lazar955 Lazar955 marked this pull request as ready for review October 10, 2024 15:53
@Lazar955 Lazar955 linked an issue Oct 10, 2024 that may be closed by this pull request
@RafilxTenfen
Copy link
Contributor

Could you share the flag error it was giving?

@Lazar955
Copy link
Member Author

Lazar955 commented Oct 10, 2024

Could you share the flag error it was giving?

@RafilxTenfen it's inheriting the flags passed, we pass in the "daemon-address" flag needed for FP CLI, which also gets passed to the Babylon command, resulting in the unexpected flag.

But anyways, I find this approach better, as you are handling things on the "server" rather than on the "client". As of now CLI just calls the RPC where everything is handled. In the other approach where the CLI would:

  1. Fetch the FP from Babylon
  2. Determining to set or not flags for the babylon edit fp cmd
  3. Invoke the cmd on babylon
  4. Still have to invoke the FP RPC, because we need to update the local store

finality-provider/cmd/fpd/daemon/daemon_commands.go Outdated Show resolved Hide resolved
finality-provider/cmd/fpd/daemon/daemon_commands.go Outdated Show resolved Hide resolved
finality-provider/cmd/fpd/daemon/daemon_commands.go Outdated Show resolved Hide resolved
clientcontroller/babylon.go Outdated Show resolved Hide resolved
clientcontroller/interface.go Outdated Show resolved Hide resolved
clientcontroller/babylon.go Show resolved Hide resolved
@Lazar955 Lazar955 requested a review from gitferry October 15, 2024 12:30
Copy link
Contributor

@RafilxTenfen RafilxTenfen left a comment

Choose a reason for hiding this comment

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

In my mind it would be easier to take advantage of the CLI that already exists on Babylon like #92 and add a post run to update the local storage of fpd if needed

But, since this is already developed and it does something more than just call the transaction (set default values) the code looks good to me

Copy link
Member

@gitferry gitferry left a comment

Choose a reason for hiding this comment

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

Lgtm! Some minor comments:

clientcontroller/interface.go Outdated Show resolved Hide resolved
itest/e2e_test.go Show resolved Hide resolved
@gitferry
Copy link
Member

Seems CI was not triggered?

@Lazar955
Copy link
Member Author

Seems CI was not triggered?

prob because of conflict with the changelog

@Lazar955 Lazar955 merged commit cee934b into main Oct 15, 2024
12 checks passed
@Lazar955 Lazar955 deleted the lazar/edit-fp-desc branch October 15, 2024 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add edit-finality-provider to edit fp description
3 participants