citadel-tech / coinswap

Functioning, minimal-viable binaries and libraries to perform a trustless, p2p Maxwell-Belcher Coinswap Protocol
https://gist.github.com/chris-belcher/9144bd57a91c194e332fb5ca371d0964
Other
73 stars 46 forks source link

`improve`: write modified config values back to the maker `config.toml` file #246

Closed wthrajat closed 3 weeks ago

wthrajat commented 2 months ago
        let mut config = MakerConfig::new(Some(&data_dir.join("config.toml")))?;

        if let Some(port) = port {
            config.port = port;
        }

        if let Some(rpc_port) = rpc_port {
            config.rpc_port = rpc_port;
        }

        if let Some(socks_port) = socks_port {
            config.socks_port = socks_port;
        }

        if let Some(connection_type) = connection_type {
            config.connection_type = connection_type;
        }

Maker config parameters like port, rpc_port, socks_port, connection_type when updated with new values, does NOT reflect in the config.toml file. This PR aims to fix that.

wthrajat commented 2 months ago

Clippy has been recently updated with a new lint: Too long first doc paragraph as described in doc_mod.rs. Pushed the fix in 7d3ff35

mojoX911 commented 2 months ago

@wthrajat can you make the long doc fix in a separate PR and I will merge it quickly. This error will trickle into other PRs too.

Shourya742 commented 2 months ago

@wthrajat Can you squeeze the commits.

mojoX911 commented 2 months ago

@wthrajat any update on this?

wthrajat commented 1 month ago

@mojoX911

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 97.56098% with 1 line in your changes missing coverage. Please review.

Project coverage is 79.83%. Comparing base (54b9299) to head (20f900f). Report is 32 commits behind head on master.

Files with missing lines Patch % Lines
src/market/directory.rs 93.33% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #246 +/- ## ========================================== + Coverage 78.33% 79.83% +1.49% ========================================== Files 30 30 Lines 6297 6342 +45 ========================================== + Hits 4933 5063 +130 + Misses 1364 1279 -85 ``` | [Flag](https://app.codecov.io/gh/citadel-tech/coinswap/pull/246/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=citadel-tech) | Coverage Δ | | |---|---|---| | [](https://app.codecov.io/gh/citadel-tech/coinswap/pull/246/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=citadel-tech) | `?` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=citadel-tech#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

wthrajat commented 3 weeks ago

@Shourya742: Regarding:

I noticed that we are still using the method from the utility.

Here we have concluded to use the utill method only, renaming it as write_toml_to_file()

Shourya742 commented 3 weeks ago

@Shourya742: Regarding:

I noticed that we are still using the method from the utility.

Here we have concluded to use the utill method only, renaming it as write_toml_to_file()

What I'm suggesting is to associate a write_to_toml method with the directory config. This would make our implementation consistent across all config structs, eliminating the need for a separate TOML utility, as I don't see any other use case for it.

mojoX911 commented 3 weeks ago

I say would its okay for now, and add that when the need arises. It will arise when we separate the crates. But its very far away in the roadmap. In the meantime I would prefer to reduce duplicate code. So a single util method for toml writing makes sense. And who knows, by that far in future, we might find more usecase for it.

It will also not be just the toml utility. Many other things will need to be localised. We will have to refactor through all of that. Lets not burden ourselves now on stuffs that far away in future. We do it when we do it. Till then better to keep architecture as per single crate design.

mojoX911 commented 3 weeks ago

Oh wait. We are already duplicating the code. We should just use the write_toml_to_file util everywhere. Or makes sense to just remove it and inline the function. Its just few lines.

wthrajat commented 3 weeks ago

@Shourya742, @mojoX911:

Upto you. I don't have strong opinion on either.

I think I will go with inlining it in the directory.rs file -- which means removing the write_toml_to_file() completely since it was only being used there.

Reason being I have a few ideas for writing the configs of maker, taker and directory as I am thinking of implementing a trait (but in another PR) for writing configs that can be used in all three structs (MakerConfig, TakerConfig and DirectoryServer) and more structs if needed. That will reduce a lot of code.