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

Cargo fmt and other misc updates #10

Closed
wants to merge 1 commit into from
Closed

Conversation

0xBEEFCAF3
Copy link

Got a working example using Wasabi. In the process of getting everything set up, I updated some docs, updated some dependency versions, gitignore, and setup a template config file. Hope others will find this useful

Copy link
Owner

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

Thanks for the PR and reporting success with Wasabi! There are some good ideas. Unfortunately you made some mistakes and I hate rustfmt formatting. Please avoid reformatting the code, I will try to configure rustfmt to produce something nicer later. I pointed out things I dislike so you could try it too but I'd prefer to do it later, in a separate PR.

Also, very important, never mix formatting commits with anything else. It makes it painful to review and it is probably no-go in other projects as well.

.gitignore Outdated
target/

# These are backup files generated by rustfmt
**/*.rs.backup
Copy link
Owner

Choose a reason for hiding this comment

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

I never seen them. Maybe you ran rustfmt on uncommitted repository?

Copy link

Choose a reason for hiding this comment

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

second, I'm not sure what this is even after trying to duplicate

tokio = { version = "1.7.1", features = ["rt-multi-thread"] }
rand = "0.8.4"
base64 = "0.13.0"
serde = "1.0.126"
serde_json = "1.0.64"
serde_derive = "1.0.126"
ln-types = { version = "0.1.2", features = ["serde", "parse_arg"] }
ln-types = { version = "0.1.3", features = ["serde", "parse_arg"] }
Copy link
Owner

Choose a reason for hiding this comment

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

This is required because of LNP-WG/ln-types@a1de886 right?

Copy link

Choose a reason for hiding this comment

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

Yes that has been my experience

README.md Outdated
- No discount possible
- Invalid request can kill whole server
- `.unwraps()`s EVERYWHERE!
- I swear I knew about a few more but can't remember right now :D
Copy link
Owner

Choose a reason for hiding this comment

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

Why? I'm used to the syntax I wrote. Are ther some guidelines against it or something?

README.md Outdated
@@ -72,9 +72,14 @@ In other words, your grandmother will be able to somewhat privately open a bunch
5. Copy BIP21 from command line output and paste it into one of the supported wallets
6. Confirm the transaction and pray it works

## Dev set up

0. Copy `conf_dir/conf.template` to `conf_dir/conf` and replace values
Copy link
Owner

Choose a reason for hiding this comment

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

Could also cargo run -- --bind-port 3000 --lnd-address https://localhost:10009 --lnd-cert-path tls..cert --lnd-macaroon-path admin.macaroon ;)

Copy link

Choose a reason for hiding this comment

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

afaik there's no man page that shows that yet

src/main.rs Outdated
use std::fmt;
use ln_types::P2PAddress;
use std::sync::{Arc, Mutex};
Copy link
Owner

Choose a reason for hiding this comment

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

Reordering is annoying but I can live with it.

src/main.rs Outdated
// let (config, mut args) =
// Config::including_optional_config_files(std::iter::empty::<&str>()).unwrap_or_exit();
let (config, mut args) =
Config::including_optional_config_files(&["conf_dir/conf"]).unwrap_or_exit();
Copy link
Owner

Choose a reason for hiding this comment

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

Even if I wanted to add a default config file, this naming is strange. loptos.conf would be better. Note that people still can use --conf option to specify custom config.

Copy link

Choose a reason for hiding this comment

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

seems like there's no man page which would help find this piece of docs

src/main.rs Outdated
.await
.expect("failed to connect");

println!("[DEBUG]: Connected to lnd");
Copy link
Owner

Choose a reason for hiding this comment

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

I'd prefer using slog but I guess it's OK for now.

src/main.rs Outdated
arg.to_str().expect("wallet amount not UTF-8"),
bitcoin::Denomination::Satoshi,
)?
}
Copy link
Owner

Choose a reason for hiding this comment

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

Missing commas are confusing.

src/main.rs Outdated
scheduled_payjoin
.total_amount()
.to_string_in(bitcoin::Denomination::Bitcoin)
);
Copy link
Owner

Choose a reason for hiding this comment

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

Amount string needs to go to a variable to not blow up the call. (Also ideally we would bump bitcoin version once it's realeased and use the newly added display_in method.)

@@ -0,0 +1,4 @@
bind_port = 3000
lnd_address = "tcp://localhost:10009"
Copy link
Owner

Choose a reason for hiding this comment

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

I will have to study what tcp: does, probably unsecure connection. Please use https instead or explain why you used tcp

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.

None yet

3 participants