Env config names and defaults, merge Chain and Network args #230

h4sh3d · 2021-11-23T11:43:10Z

Shortify configuration names
Merge lnpbp Chain and Farcaster Network
others minor changes

codecov-commenter · 2021-11-23T11:48:16Z

Codecov Report

Merging #230 (2d51b21) into main (04b951f) will decrease coverage by 0.1%.
The diff coverage is 3.2%.

@@           Coverage Diff           @@
##            main    #230     +/-   ##
=======================================
- Coverage   15.2%   15.1%   -0.1%     
=======================================
  Files         27      27             
  Lines       6371    6472    +101     
=======================================
+ Hits         970     979      +9     
- Misses      5401    5493     +92

Impacted Files	Coverage Δ
src/config.rs	`0.0% <0.0%> (-50.0%)`	⬇️
src/error.rs	`0.0% <0.0%> (-1.8%)`	⬇️
src/farcasterd/opts.rs	`0.0% <0.0%> (ø)`
src/farcasterd/runtime.rs	`0.0% <0.0%> (ø)`
src/lib.rs	`13.2% <0.0%> (+3.8%)`	⬆️
src/peerd/opts.rs	`0.0% <ø> (ø)`
src/peerd/runtime.rs	`0.0% <ø> (ø)`
src/swapd/opts.rs	`0.0% <0.0%> (ø)`
src/swapd/runtime.rs	`0.0% <0.0%> (ø)`
src/syncerd/bitcoin_syncer.rs	`0.0% <0.0%> (ø)`
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 04b951f...2d51b21. Read the comment docs.

zkao · 2021-11-23T12:49:17Z

the configuration chain is not used, afaik

zkao · 2021-11-23T12:49:39Z

and shall be removed

h4sh3d · 2021-11-23T13:11:56Z

The configuration network is used in the syncers. And will be used while making an offer in my next PR.

zkao · 2021-11-23T15:02:10Z

The configuration network is used in the syncers. And will be used while making an offer in my next PR.

it should not be possible to use global arguments for syncers, because farcasterd might launch 10 different syncers

zkao · 2021-11-23T15:02:57Z

running for different coins on different sub networks

TheCharlatan · 2021-11-23T15:04:08Z

I think if I understood @h4sh3d correctly, the configuration is relegated back to whatever is in the offer in his next PR.
EDIT: On seconds thought, you would still need a separate argument for the network the for each syncer, right?

zkao · 2021-11-23T15:05:19Z

@TheCharlatan, can u shed some light on this, since u worked on the syncers config?

but my understanding is that chain shall not be a global config, isnt it?

h4sh3d · 2021-11-23T15:05:24Z

Uhm, not really. I thought about a setup where if launched with --testnet farcasterd cannot do mainnet swap in parallel. Do we really want to mix everything (in term of network)?

EDIT: replying to @TheCharlatan

TheCharlatan · 2021-11-23T15:06:36Z

Then I misunderstood the purpose @h4sh3d , I agree with @zkao then that farcasterd should give this argument to the syncer manually.

h4sh3d · 2021-11-23T15:08:11Z

Then I misunderstood the purpose @h4sh3d , I agree with @zkao then that farcasterd should give this argument to the syncer manually.

It does, but through the shared flags and not with two parallel values.

So we want to allow running swaps on different networks with the same farcasterd instance?

zkao · 2021-11-23T15:09:46Z

a Syncer is:

farcaster-node/src/service.rs

Line 104 in 183398d

Syncer(Coin, Network),

h4sh3d · 2021-11-23T15:11:49Z

Yes, obviously. So farcasterd will be able to manage a list of Syncers by tuple (Coin, Network), if I do a make --testnet btc xmr it will check and launch both testnet syncer for btc and xmr, if 1 minute after I do make --mainnet btc xmr it will launch the mainnet ones, correct?

zkao · 2021-11-23T15:24:42Z

The obvious thing: syncer config should be local (independent of whether farcasterd mixes networks). <- this is a bug

Farcasterd mixing or not networks is an opinionated detail that has no urgency <- this is an opinion

zkao · 2021-11-23T15:55:38Z

merging Coin with the constructs used in core would also be appreciated

zkao · 2021-11-23T15:59:23Z

the hard bit is how to make these farcasterd options disappear, any thought, @h4sh3d and @TheCharlatan ?

--electrum-server localhost:60001 --monero-daemon http://localhost:38081 --monero-rpc-wallet http://localhost:18083

h4sh3d · 2021-11-23T15:59:27Z

merging Coin with the constructs used in core would also be appreciated

Yes, I'll open an issue to track that and include it in core.

h4sh3d · 2021-11-23T16:01:55Z

the hard bit is how to make these farcasterd options disappear, any thought, @h4sh3d and @TheCharlatan ?

--electrum-server localhost:60001 --monero-daemon http://localhost:38081 --monero-rpc-wallet http://localhost:18083

This has to be injected in the config, if we move them out of shared opts like the network syncer could find them through the farcaster.toml config file in the data dir.

And them we extend the config to have one of each per network. I had the problem some days ago when network was testnet but a syncer was connected to a mainnet daemon, no errors were given I just found it in the middle of the swap because the height printed in the logs were weird.

h4sh3d · 2021-11-23T17:47:54Z

In my last commit (WIP) I use the config flag in args to parse a toml file containing the syncer configuration in syncerd.

zkao · 2021-11-23T23:18:59Z

src/farcasterd/runtime.rs

        }
        if !self.syncers.contains_key(&(coin_acc.clone(), network)) {
-            launch("syncerd", &[coin_acc.to_string(), network.to_string()])?;


if i were you, i would leave the stuff that is written to the toml to something runtime independent, such as a map btw say

(Monero, Testnet) -> "--monero-daemon http://localhost:38081 --monero-rpc-wallet http://localhost:18083" (Bitcoin, Testnet) -> "--electrum-server localhost:60001"

Then if the config doesnt have it, it can't run the swap.

TheCharlatan · 2021-11-26T19:05:05Z

src/bin/syncerd.rs


+fn main() -> Result<(), Error> {


What's up with the return type?

What is the problem about returning an error?

This function doesn't return, right?

The binary returns eventually, so exit code is not zero and error is printed. You can have main return result in rust :)

When I mean eventually: it returns early if it miss some params to start

The binary returns eventually, so exit code is not zero and error is printed. You can have main return result in rust :)

But which statement within this main function actually returns? I only see a panic case here, and no ?, return or omitted ;. So I'm guessing you are just returning for style? But even then, it seems inconsistent with swapd.rs and peerd.rs.

Oh sorry, I guess the code that was there got removed, run fn can panic but it is handled by the expect so yes I have to removed that. (On the phone, didn’t open the file before now to check)

Done in 9e3a5fe

TheCharlatan · 2021-11-26T19:23:19Z

src/config.rs

+
+#[derive(Deserialize, Serialize, Debug, Clone)]
+#[serde(crate = "serde_crate")]
+pub struct Syncers {


Syncers is too generic a name for my taste, especially since we have other syncer vars in the code. How about just SyncerConfig

Done in 9e3a5fe

…ing args per coin

h4sh3d · 2021-11-29T13:54:21Z

Fixing clippy...

TheCharlatan

tACK

h4sh3d marked this pull request as draft November 23, 2021 11:50

h4sh3d marked this pull request as ready for review November 23, 2021 13:38

TheCharlatan approved these changes Nov 23, 2021

View reviewed changes

h4sh3d mentioned this pull request Nov 23, 2021

Adding Coin from node farcaster-project/farcaster-core#180

Closed

h4sh3d marked this pull request as draft November 23, 2021 17:45

zkao reviewed Nov 23, 2021

View reviewed changes

h4sh3d force-pushed the minor-changes-batch-1 branch 2 times, most recently from 7a3fd5c to 35b0e6a Compare November 24, 2021 20:41

TheCharlatan mentioned this pull request Nov 25, 2021

Swap functional test #231

Merged

h4sh3d force-pushed the minor-changes-batch-1 branch from 0094f59 to 349d860 Compare November 26, 2021 15:32

h4sh3d marked this pull request as ready for review November 26, 2021 16:28

TheCharlatan reviewed Nov 26, 2021

View reviewed changes

TheCharlatan mentioned this pull request Nov 26, 2021

Syncer TaskId #206

Merged

TheCharlatan self-requested a review November 27, 2021 09:37

h4sh3d added 18 commits November 29, 2021 14:38

Format bitcoin amount in logs as BTC

4d23f44

Change default electrum server, fix farcaster-project#219

08b78cf

Shortify ENV variable names

4559fef

Simplify network and chain argument

36cedf3

Use local network parameter for syncers

6a9ed7a

Adapt command line argument of bins, add farcasterd config file

8ae0668

Remove default syncer URLs, add method to extract part of config

d4500f3

Forward config value to syncerd cmd line arguments, only minimal work…

fda27f9

…ing args per coin

Start syncers with minimal set of URLs only

87bffaf

Allow failaible start for synclets

843ceb0

Fix default config values

5f80663

Modify rpc tests

8b8a358

Try to fix functional tests

7819bcb

Remove env log init, add 5min timeout to tests

de58289

Rollback bitcoin versions

b6d4393

Ran cargo fmt

78e306e

Fix clippy errors

bf12a8f

Fix farcasterd config exemple

c134876

h4sh3d force-pushed the minor-changes-batch-1 branch from 1256d8d to 9e3a5fe Compare November 29, 2021 13:47

Address comments from @TheCharlatan

9e3a5fe

h4sh3d added 3 commits November 29, 2021 14:57

Fix clippy errors

a40c71e

Fix sleep statement to let electrum catch up state

b298d12

Add local syncer in config

2d51b21

TheCharlatan approved these changes Nov 29, 2021

View reviewed changes

TheCharlatan merged commit 960478e into farcaster-project:main Nov 29, 2021

TheCharlatan mentioned this pull request Nov 30, 2021

Default arguments values uses different networks #229

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Env config names and defaults, merge Chain and Network args #230

Env config names and defaults, merge Chain and Network args #230

h4sh3d commented Nov 23, 2021

codecov-commenter commented Nov 23, 2021 •

edited

Loading

zkao commented Nov 23, 2021

zkao commented Nov 23, 2021

h4sh3d commented Nov 23, 2021 •

edited

Loading

zkao commented Nov 23, 2021

zkao commented Nov 23, 2021

TheCharlatan commented Nov 23, 2021 •

edited

Loading

zkao commented Nov 23, 2021

h4sh3d commented Nov 23, 2021 •

edited

Loading

TheCharlatan commented Nov 23, 2021

h4sh3d commented Nov 23, 2021 •

edited

Loading

zkao commented Nov 23, 2021

h4sh3d commented Nov 23, 2021 •

edited

Loading

zkao commented Nov 23, 2021

zkao commented Nov 23, 2021

zkao commented Nov 23, 2021 •

edited

Loading

h4sh3d commented Nov 23, 2021

h4sh3d commented Nov 23, 2021 •

edited

Loading

h4sh3d commented Nov 23, 2021 •

edited

Loading

zkao Nov 23, 2021

TheCharlatan Nov 26, 2021

h4sh3d Nov 26, 2021

TheCharlatan Nov 26, 2021

h4sh3d Nov 26, 2021

h4sh3d Nov 26, 2021

TheCharlatan Nov 26, 2021

h4sh3d Nov 26, 2021

h4sh3d Nov 29, 2021

TheCharlatan Nov 26, 2021 •

edited

Loading

h4sh3d Nov 29, 2021

h4sh3d commented Nov 29, 2021

TheCharlatan left a comment

Env config names and defaults, merge Chain and Network args #230

Env config names and defaults, merge Chain and Network args #230

Conversation

h4sh3d commented Nov 23, 2021

codecov-commenter commented Nov 23, 2021 • edited Loading

Codecov Report

zkao commented Nov 23, 2021

zkao commented Nov 23, 2021

h4sh3d commented Nov 23, 2021 • edited Loading

zkao commented Nov 23, 2021

zkao commented Nov 23, 2021

TheCharlatan commented Nov 23, 2021 • edited Loading

zkao commented Nov 23, 2021

h4sh3d commented Nov 23, 2021 • edited Loading

TheCharlatan commented Nov 23, 2021

h4sh3d commented Nov 23, 2021 • edited Loading

zkao commented Nov 23, 2021

h4sh3d commented Nov 23, 2021 • edited Loading

zkao commented Nov 23, 2021

zkao commented Nov 23, 2021

zkao commented Nov 23, 2021 • edited Loading

h4sh3d commented Nov 23, 2021

h4sh3d commented Nov 23, 2021 • edited Loading

h4sh3d commented Nov 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheCharlatan Nov 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h4sh3d commented Nov 29, 2021

TheCharlatan left a comment

Choose a reason for hiding this comment

codecov-commenter commented Nov 23, 2021 •

edited

Loading

h4sh3d commented Nov 23, 2021 •

edited

Loading

TheCharlatan commented Nov 23, 2021 •

edited

Loading

h4sh3d commented Nov 23, 2021 •

edited

Loading

h4sh3d commented Nov 23, 2021 •

edited

Loading

h4sh3d commented Nov 23, 2021 •

edited

Loading

zkao commented Nov 23, 2021 •

edited

Loading

h4sh3d commented Nov 23, 2021 •

edited

Loading

h4sh3d commented Nov 23, 2021 •

edited

Loading

TheCharlatan Nov 26, 2021 •

edited

Loading