-
Notifications
You must be signed in to change notification settings - Fork 919
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
refactor!(params): Moving params to nodebuilder and adding config #1168
refactor!(params): Moving params to nodebuilder and adding config #1168
Conversation
00c4075
to
6667c7a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, I like the idea of making params
as a module or a part of a module What if we go one step further and make it part of the node
module? node.Network
, node.Bootstrapper
, etc. I don't think network
deserves its own module, just for 3 types there, but could be a coherent addition to the node module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that i like params living inside nodebuilder/node
I think of node
as administrative -- would you be opposed to a nodebuilder/netparams
package instead?
These params could honestly even live inside of p2p module bc they are related to p2p. |
I wouldnt be opposed to that, I originally had them in their own package - @Wondertan what do you think? |
I agree that |
Some issues came up during rebasing, fixing |
8b88e26
to
aaf719b
Compare
aaf719b
to
241335f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requesting changes because the solution for #1189 in this PR does not fix the core problem of #1189: our libs/pkgs should not import node and its modules, as it is a logical cyclic dependency(node depends on header and wise-versa) and prevents usage of header pkg without importing the node.
The alternative is passing the network name as a protocol prefix string which is better be optional. Our protocols will be used in other projects. Specifically, the header pkg and there is already ongoing work in https://github.com/celestiaorg/go-header
Let's revert the change and solve the #1189 issue in the subsequent PR, as it is out of scope. It's fine that the main will be broken for some time for Mamaki.
241335f
to
1ed5c77
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome
1ed5c77
to
ee70a28
Compare
@distractedm1nd, conflicts |
i literally rebased this morning 🥲 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note that this PR breaks support connection to the non-default networks. Perhaps, we should not merge it for the soonish upcoming release, so we have enough time to fix the issue correctly right after merging it
ee70a28
to
0cab3c4
Compare
0cab3c4
to
d7b8d37
Compare
@Wondertan as long as celestia-node requires header-ex and fraud-ex to contain appended chainIDs, I'm fine with this approach. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two things to note for clarity (which have already been noted but good to repeat for doc reasons):
- This PR is breaking for all networks besides
arabica
-- if you try to run with--p2p.network mamaki
, it will not run against mamaki unless mamaki bootstrappers are upgraded, in which case, they can run but they'll be running on the sameheader-ex
protocol ID asarabica
which is potentially wonky (in the case that a mamaki node accidentally sets an arabica bootstrapper or something, it could try to sync blocks from arabica node).
We need to get a follow-up PR addressing #1189 preferably soon after this is merged (or at least get a PR based on this one out already so we can merge one after the other).
- This PR will break CLI as well:
node.network
-->p2p.network
.
Codecov Report
@@ Coverage Diff @@
## main #1168 +/- ##
==========================================
+ Coverage 55.86% 56.14% +0.27%
==========================================
Files 160 160
Lines 9725 9932 +207
==========================================
+ Hits 5433 5576 +143
- Misses 3756 3802 +46
- Partials 536 554 +18
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking through this again. I love the idea so far. Making Bootstrappers/Network/...
a module- and application-specific types(independent from our pkgs/libs) fits very well with the modularity vision. Thanks, @distractedm1nd!
Q: Should we keep the main broken for network switching, or should we implement a fix on top of this PR first and then merge everything together? The second approach seems better in case some other bug appears or so.
Closes #1076
Reviewers: Please test for yourself to verify it still works.
@Wondertan what do you think of this approach?
In the style of #1161