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.

Sign up for GitHub

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

Jump to bottom

[CORE-715] Bridge Daemon: Do not require eth rpc endpoint. Instead panic on startup #725

Merged
merged 1 commit into from
Oct 30, 2023
Merged

[CORE-715] Bridge Daemon: Do not require eth rpc endpoint. Instead panic on startup #725

merged 1 commit into from
Oct 30, 2023

Conversation

tqin7
Copy link
Contributor

@tqin7 tqin7 commented Oct 30, 2023 •
edited
Loading

Changelist

On bridge daemon startup, return an error if eth rpc endpoint is empty (which app.go will panic on)

Validator

  • fine if eth rpc endpoint flag is set
  • panics otherwise

Full node

  • fine if --non-validating-full-node = true (or if the flag is set)
  • panics otherwise

Test Plan

[Describe how this PR was tested (if applicable)]

Author/Reviewer Checklist

  • If this PR has changes that result in a different app state given the same prior state and transaction list, manually add the state-breaking label.
  • If this PR isn't state-breaking but has changes that modify behavior in PrepareProposal or ProcessProposal, manually add the label proposal-breaking.
  • If this PR is one of many that implement a specific feature, manually label them all feature:[feature-name].
  • If you wish to for mergify-bot to automatically create a PR to backport your change to a release branch, manually add the label backport/[branch-name].
  • Manually add any of the following labels: refactor, chore, bug.

@linear
Copy link

linear bot commented Oct 30, 2023

CORE-722 bridge daemon - not require eth rpc flag and panic on startup if flag is empty

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 30, 2023 •
edited
Loading

Walkthrough

The changes primarily focus on the handling of the --bridge-daemon-eth-rpc-endpoint flag in the bridge daemon. The requirement for this flag has been removed from the AddDaemonFlagsToCmd function, but a check has been added in the client to ensure it is set before the application starts.

Changes

File Path Summary
protocol/daemons/bridge/client/client.go Added a check to ensure the --bridge-daemon-eth-rpc-endpoint flag is set before the application starts.
protocol/daemons/flags/flags.go Removed the requirement for the FlagBridgeDaemonEthRpcEndpoint flag in the AddDaemonFlagsToCmd function.
Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • If you reply to a review comment from CodeRabbit, the bot will automatically respond.
  • To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
  • Note: Review comments are made on code diffs or files, not on the PR overview.
  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai help to get help.
  • @coderabbitai resolve to resolve all the CodeRabbit review comments.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.json

@tqin7 tqin7 changed the title bridge daemon does not require eth rpc endpoint flag and panics on startup if flag is not set [CORE-722] bridge daemon does not require eth rpc endpoint flag and panics on startup if flag is not set Oct 30, 2023
coderabbitai[bot]
coderabbitai bot reviewed Oct 30, 2023
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 8261625 and 037f865.
Files selected for processing (2)
  • protocol/daemons/bridge/client/client.go (1 hunks)
  • protocol/daemons/flags/flags.go (1 hunks)
Files skipped from review due to trivial changes (1)
  • protocol/daemons/flags/flags.go

protocol/daemons/bridge/client/client.go Outdated Show resolved Hide resolved
@tqin7 tqin7 changed the title [CORE-722] bridge daemon does not require eth rpc endpoint flag and panics on startup if flag is not set [CORE-715] bridge daemon does not require eth rpc endpoint flag and panics on startup if flag is not set Oct 30, 2023
coderabbitai[bot]
coderabbitai bot reviewed Oct 30, 2023
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 8261625 and 3e0b045.
Files selected for processing (2)
  • protocol/daemons/bridge/client/client.go (2 hunks)
  • protocol/daemons/flags/flags.go (1 hunks)
Files skipped from review due to trivial changes (1)
  • protocol/daemons/flags/flags.go
Additional comments: 2
protocol/daemons/bridge/client/client.go (2)

  • 37-43: The check for EthRpcEndpoint being empty is a good addition. It ensures that the application will not start without a valid Ethereum RPC endpoint, which is a required parameter for the bridge daemon to function correctly. This is a good practice for error handling and improves the robustness of the application.

  • 37-46: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [46-48]

Error handling is done correctly here. If there is an error while establishing a connection, the error is logged and returned immediately. This is a good practice as it prevents the application from proceeding with an invalid state.

protocol/daemons/bridge/client/client.go Outdated Show resolved Hide resolved
tqin7
tqin7 commented Oct 30, 2023
View reviewed changes
protocol/daemons/bridge/client/client.go
@@ -36,6 +37,10 @@ func Start(
"Starting bridge daemon with flags",
"BridgeFlags", flags.Bridge,
)
// Panic if EthRpcEndpoint is empty.
if flags.Bridge.EthRpcEndpoint == "" {
return fmt.Errorf("flag %s is not set", daemonflags.FlagBridgeDaemonEthRpcEndpoint)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

app.go will panic on this error

@tqin7 tqin7 added the backport/protocol/v1.x Backport v1.x label Oct 30, 2023
coderabbitai[bot]
coderabbitai bot reviewed Oct 30, 2023
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 8261625 and a6b01e0.
Files selected for processing (2)
  • protocol/daemons/bridge/client/client.go (3 hunks)
  • protocol/daemons/flags/flags.go (1 hunks)
Files skipped from review due to trivial changes (2)
  • protocol/daemons/bridge/client/client.go
  • protocol/daemons/flags/flags.go

@ttl33 ttl33 changed the title [CORE-715] bridge daemon does not require eth rpc endpoint flag and panics on startup if flag is not set [CORE-715] Bridge Daemon: Do not require eth rpc endpoint. Instead panic on startup Oct 30, 2023
ttl33
ttl33 approved these changes Oct 30, 2023
View reviewed changes
Copy link
Contributor

@ttl33 ttl33 left a comment

Choose a reason for hiding this comment

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

Can we add unit or e2e testing retroactively? (non-blocking for this PR submission)

@tqin7 tqin7 merged commit 17e3c37 into main Oct 30, 2023
13 of 16 checks passed
@tqin7 tqin7 deleted the tq/core-722 branch October 30, 2023 21:00
mergify bot pushed a commit that referenced this pull request Oct 30, 2023
@tqin7 @mergify
bridge daemon does not require eth rpc endpoint flag and panics on st…
250b9da 
…artup if flag is not set (#725)

(cherry picked from commit 17e3c37)
@mergify mergify bot mentioned this pull request Oct 30, 2023
Merged
tqin7 added a commit that referenced this pull request Oct 30, 2023
@mergify @tqin7
bridge daemon does not require eth rpc endpoint flag and panics on st…
d08c8e0 
…artup if flag is not set (#725) (#727)

(cherry picked from commit 17e3c37)

Co-authored-by: Tian <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

@ttl33 ttl33 ttl33 approved these changes

Assignees
No one assigned
Labels
backport/protocol/v1.x Backport v1.x protocol
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tqin7 @ttl33