这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@SebastianElvis
Copy link
Member

@SebastianElvis SebastianElvis commented Jul 15, 2025

Part of #507

This PR generalises the implementation of existing CLIs for the finality provider, and leverage the generalised utilities to implement the CLIs for rollup BSNs. In particular, this PR

  • classifies the type of CLIs into the following
    • common commands (under finality-provider/cmd/fpd/common/common_cmd.go), shared among any type of FPs
    • incentive related commands (under finality-provider/cmd/fpd/common/incentive_cmd.go), shared among any type of FPs
    • key related commands (under finality-provider/cmd/fpd/common/keys_cmd.go), shared among any type of FPs
    • version commands (under version/), shared among any type of FPs
    • daemon commands (under finality-provider/cmd/fpd/daemon), that are specific to each type of FPs
  • introduces some helper functions and simplifies the construction of the root command
  • refactors daemon commands under finality-provider/cmd/fpd/daemon such that most of the code is reusable for other types of FPs
    • The main distinction of daemon commands between different FPs is the definition of the config object, and the construction of FinalityProviderApp. The refactoring allows having different logics for this part whereas the other parts remain reusable.
  • uses the utilities under finality-provider/cmd/fpd/daemon to construct the daemon cmds for rollup FPs.

This is a major refactoring to simplify the development of the BSN finality providers.

Some future work in subsequent PRs:

  • There is still space for further simplification (especially making it even easier to develop BSN-specific daemon commands).
  • Introduce e2e tests for cmds
  • Make utilities in e2e tests reusable to simplify e2e tests for BSN FPs

@SebastianElvis SebastianElvis changed the title cli: refactor CLIs and generalise for rollup FPs cli: generalise CLIs and introduce CLIs for rollup FP Jul 15, 2025
@SebastianElvis SebastianElvis marked this pull request as ready for review July 15, 2025 06:11
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors and generalizes the CLI layer for finality providers and applies the new abstractions to implement a rollup BSN CLI.

  • Extract common, keys, incentive, and version subcommands into reusable packages
  • Refactor daemon commands into templates and shared utilities
  • Add a rollup-specific CLI by wiring the generalized commands

Reviewed Changes

Copilot reviewed 26 out of 26 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
version/cmd.go Introduce AddVersionCommands helper and tidy imports
types/txresponse.go Add PrintRespJSON helper for pretty-printed JSON responses
finality-provider/cmd/fpd/main.go Rewrite root command to use generic Add*Commands calls
finality-provider/cmd/fpd/daemon/start.go Separate template and execution for the start subcommand
bsn/rollup-finality-provider/cmd/rollup-fpd/main.go Bootstraps rollup CLI using the shared command packages
Comments suppressed due to low confidence (2)

finality-provider/cmd/fpd/main.go:40

  • [nitpick] New generic CLI groups are registered here but there are no unit tests verifying that the expected subcommands are present. Consider adding tests for AddCommonCommands to ensure all shared commands are wired correctly.
	commoncmd.AddCommonCommands(cmd)

bsn/rollup-finality-provider/cmd/rollup-fpd/main.go:40

  • [nitpick] The rollup CLI now reuses shared command groups but lacks tests to confirm correct registration. Adding tests around NewRootCmd for rollup-fpd would improve confidence.
	commoncmd.AddCommonCommands(cmd)

SebastianElvis and others added 3 commits July 15, 2025 16:17
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copy link
Member

@Lazar955 Lazar955 left a comment

Choose a reason for hiding this comment

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

Overall lgtm, minor comments, let's wait for one more review

Copy link
Contributor

@KonradStaniec KonradStaniec left a comment

Choose a reason for hiding this comment

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

nothing blocking from me :) lets just wrap errors when necessary it will help us later with all the problems

@SebastianElvis
Copy link
Member Author

SebastianElvis commented Jul 16, 2025

will wait for @0xAleksaOpacic to test out the rollup-fpd binary in local deployment before merging

UPDATE: working properly (except for known issues in fpd) now 🎉

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.

4 participants