Skip to content
This repository was archived by the owner on Oct 31, 2024. It is now read-only.

feat: add no-edge-process flag to node init #401

Merged
merged 1 commit into from
Dec 12, 2023
Merged

feat: add no-edge-process flag to node init #401

merged 1 commit into from
Dec 12, 2023

Conversation

gruberb
Copy link
Contributor

@gruberb gruberb commented Dec 11, 2023

Description

There are use cases for which we don't rely on polygon-edge. If we don't need it, we can run node up --no-edge-process. However, when creating a new config setup, we always assume polygon-edge is present. In the case of a solo-sequencer, we want to supply a fixed validator.key for example, and don't need or want polygon-edge in the process.

Fixes # (issue)

New feature

Adding a --no-edge-process flag to node init.

PR Checklist:

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added or updated tests that comprehensively prove my change is effective or that my feature works

@gruberb gruberb requested a review from a team as a code owner December 11, 2023 16:07
@gruberb gruberb requested review from hadjiszs and JDawg287 December 11, 2023 16:07
Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

LGTM

I do think that the API would have been cleaner if we had flipped the logic, to have --with-edge rather than --no-something-something. That choice was taken way back though and is a bit messy to revert so not going to complain about it now.

Another tak-shave-y consideration is naming: perhaps it would have been wise to avoid naming "Edge" and instead pick something generic, like chain-client or subnet-node or something that reflects the role it plays in topos, rather than naming a specific implementation of evm chain.

Copy link

codecov bot commented Dec 12, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (60f873c) 62.87% compared to head (c9f61a8) 62.95%.
Report is 2 commits behind head on main.

Files Patch % Lines
crates/topos/src/components/node/commands/init.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #401      +/-   ##
==========================================
+ Coverage   62.87%   62.95%   +0.07%     
==========================================
  Files         220      220              
  Lines       11760    11788      +28     
==========================================
+ Hits         7394     7421      +27     
- Misses       4366     4367       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gruberb gruberb merged commit 28a553b into main Dec 12, 2023
@gruberb gruberb deleted the feat/TP-817 branch December 12, 2023 12:40
@gruberb
Copy link
Contributor Author

gruberb commented Dec 12, 2023

Another tak-shave-y consideration is naming: perhaps it would have been wise to avoid naming "Edge" and instead pick something generic, like chain-client or subnet-node or something that reflects the role it plays in topos, rather than naming a specific implementation of evm chain.

Agree. But part of me also thinks that we can very explicit of what this command does (we just have edge for now, and probably for a longer time?). So once this is changing, we can adapt the naming again. I rather be precise, and change it for when we want to expand.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants