refactor(node): remove sequencer support (and other refactors) - #3892
refactor(node): remove sequencer support (and other refactors)#3892rodrodros wants to merge 5 commits into
Conversation
|
Claude finished @rodrodros's task in 8m 0s —— View job Review:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
- Remove sequencing support from the node for now - Move the config file to its own folder - Move the RPC servers logic to it's own script - Move local databse to script - Modified metrics with the option patter to reduce some ifs chore: minor improvs chore: improvements
|
Claude finished @rodrodros's task in 6m 48s —— View job Re-review after force-pushThe branch was amended and force-pushed (same 4 commit subjects, new SHAs). I diffed the new head against Fixed since last review
Also re-verified as equivalent to the original: the Still openImportant
Nit
Acknowledged as author decisions, not re-raised: keeping Nothing here blocks merging. · branch |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3892 +/- ##
==========================================
+ Coverage 75.26% 75.69% +0.42%
==========================================
Files 443 445 +2
Lines 40058 39864 -194
==========================================
+ Hits 30150 30174 +24
+ Misses 7806 7598 -208
+ Partials 2102 2092 -10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
* fix(feeder): timeouts don't crash on whitespace input * tests(clients/feeder): more timeout cases
infrmtcs
left a comment
infrmtcs
left a comment
There was a problem hiding this comment.
Please feel free to ignore the comments if any of them is too nit!
|
|
||
| currentTimeout := timeouts.GetCurrentTimeout() | ||
| if currentTimeout >= mediumGrowThreshold { | ||
| if currentTimeout >= 2*time.Minute { |
There was a problem hiding this comment.
I think keeping the constant make it easier to see and change it, and the name, while not the best name possible, can give us some ideas about what it is, while 2*time.Minute doesn't auto translate to a meaningful reason.
| DisableL1Verification: true, | ||
| SubmittedTransactionsCacheEntryTTL: time.Second, | ||
| }, "v0.1", logLevel) | ||
| _, err = node.New(t.Context(), |
There was a problem hiding this comment.
nit: I think t.Context() should be in its own line.
| // Sequencer support was removed and no CLI flag is registered for this | ||
| // key. It is still decoded from config files so that stale sequencer | ||
| // configs fail loudly in New instead of silently running a full node. | ||
| Sequencer bool `mapstructure:"seq-enable"` |
There was a problem hiding this comment.
I think instead of keeping this in the config, we can use MarkDeprecated instead (see "Deprecating a flag or its shorthand" in https://pkg.go.dev/github.com/spf13/pflag#section-readme).
We can also use MarkHidden to hide it from user.
| const upgraderDelay = 5 * time.Minute | ||
| const githubAPIUrl = "https://api.github.com/repos/NethermindEth/juno/releases/latest" | ||
| const latestReleaseURL = "https://github.com/NethermindEth/juno/releases/latest" |
There was a problem hiding this comment.
Why do we move the constants here?
There was a problem hiding this comment.
Same idea of scoping them to where they are used. Global constants for for global usage. I believe is more tidy this way
| if !node.cfg.DisableL1Verification { | ||
| // Due to mutually exclusive flag we can do the following. | ||
| if n.cfg.EthNode == "" { | ||
| return nil, fmt.Errorf("ethereum node address not found; Use --disable-l1-verification flag if L1 verification is not required") | ||
| if node.cfg.EthNode == "" { | ||
| return nil, fmt.Errorf("ethereum node address not set; " + | ||
| "Use --disable-l1-verification flag if L1 verification is not required", | ||
| ) |
There was a problem hiding this comment.
Since we're touching this, I think we can use this instead:
https://pkg.go.dev/github.com/spf13/cobra#Command.MarkFlagsOneRequired
|
|
||
| // History pruning needs an L1-finalised cutoff to know which blocks are | ||
| // safe to drop. If no L1 client is given, new cutoffs cannot be set. | ||
| if cfg.Prune && cfg.DisableL1Verification { |
There was a problem hiding this comment.
Since we're touching this, I think we should use this instead:
https://pkg.go.dev/github.com/spf13/cobra#Command.MarkFlagsMutuallyExclusive
2 participants
No description provided.