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.

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

move chain_config to separate crate #635

Merged
merged 13 commits into from
Sep 27, 2022

Conversation

leviathanbeak
Copy link
Contributor

@leviathanbeak leviathanbeak commented Sep 21, 2022

closes #603

Mostly copy/pasta apart from one important change, since ChainConfig was tightly coupled with Database (Db was passed during initialization of ChainConfig), I have created a ChainConfigDb trait which I passed instead. And on the fuel-core/Database side I have implemented that trait so that it is compatible for ChainConfig initialization.

By "implemented" I mean the methods were already previously implemented so I just referenced existing methods in trait implementation.

@leviathanbeak leviathanbeak changed the title move chain_config to seprate crate move chain_config to separate crate Sep 21, 2022
@leviathanbeak leviathanbeak requested a review from a team September 21, 2022 20:19
@ControlCplusControlV
Copy link
Contributor

Would it be good to have Relayer and p2p config cli commands moved into a config crate as well. That way all the configuration and parameters to run the network could be in a single place. I'm just curious if this is something we'd want to add into this new crate (which may then require a more general rename).

But I have no problems on the impl, LGTM overall and can approve if there is a reason for against a central config crate with those additional options

@leviathanbeak
Copy link
Contributor Author

leviathanbeak commented Sep 22, 2022

@ControlCplusControlV IMO the cli config commands should stay with the CLI app (unless whole CLI app gets moved). These config constructs that I've moved out are mostly that someone (ie SDK) can import them without importing whole of fuel-core on the other hand, I don't see that being the case with CLI constructs which are just there to be constructed from the cli commands.

Copy link
Contributor

@ControlCplusControlV ControlCplusControlV left a comment

Choose a reason for hiding this comment

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

LGTM, would approve but will wait for 1 other client team member to confirm, and didn't want to take away the client team review notif

@Voxelot
Copy link
Member

Voxelot commented Sep 23, 2022

Missing README.md for new package

@leviathanbeak
Copy link
Contributor Author

@Voxelot please re-review 🎉

@Voxelot
Copy link
Member

Voxelot commented Sep 23, 2022

reserved the name on crates.io: https://crates.io/crates/fuel-chain-config

@Voxelot
Copy link
Member

Voxelot commented Sep 23, 2022

Maybe we should organize this new crate a bit more and move the different structs/traits into their own submodule files? I'm finding it easy to get lost in this large lib.rs.

@leviathanbeak
Copy link
Contributor Author

@Voxelot good point, I'll do it

@leviathanbeak
Copy link
Contributor Author

I've moved config structs into separate files: config/{struct_name}.rs and exported them all

@leviathanbeak leviathanbeak self-assigned this Sep 26, 2022
@@ -19,6 +19,7 @@ harness = true
[dependencies]
async-std = "1.12"
chrono = { version = "0.4", features = ["serde"] }
fuel-chain-config = { path = "../fuel-chain-config", version = "0.1.0" }
Copy link
Member

Choose a reason for hiding this comment

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

use re-exported config from fuel-core instead of making a new dep here.

@@ -0,0 +1,27 @@
[package]
name = "fuel-chain-config"
version = "0.1.0"
Copy link
Member

@Voxelot Voxelot Sep 26, 2022

Choose a reason for hiding this comment

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

version must match the other crates in lockstep to pass publish tag verification during the publish ci job

Suggested change
version = "0.1.0"
version = "0.10.1"

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.

Relocate ChainConfig types to a separate crate within fuel-core
3 participants