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

Simplifies the cln plugin option parsing #170

Merged
merged 2 commits into from
Jan 11, 2023

Conversation

sr-gi
Copy link
Member

@sr-gi sr-gi commented Jan 3, 2023

Options include helper functions to convert them to their proper type after
cln-plugin=0.1.2. Use that to reduce the option parsing boilerplate.

Also move all names, descriptions and default values for options, rpc_methods and hooks
to a new file.

@sr-gi sr-gi added Seeking Code Review review me pls cln-plugin Stuff related to watchtower-plugin easy to review easypeasy labels Jan 3, 2023
@sr-gi sr-gi force-pushed the cln-simplify-options branch from a1bdf26 to fba958a Compare January 3, 2023 11:54
Copy link
Collaborator

@mariocynicys mariocynicys left a comment

Choose a reason for hiding this comment

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

A couple of nits, all fine otherwise.

}

#[derive(Clone, Serialize, Debug, PartialEq, Eq)]

Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: an empty line here.

Comment on lines 71 to 73
if !host.starts_with("http") {
host = format!("http://{}", host)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: host isn't quite the name for an HTTP url.
Also let's check .starts_with("http://") because a host name might actually start with http 😆.

@mariocynicys
Copy link
Collaborator

Ok now it makes sense why the PR's name is not very inclusive.
I think you included changes from #160 in here by mistake in 4c9141f.

@sr-gi
Copy link
Member Author

sr-gi commented Jan 5, 2023

Oh yeah, this is not pulling from master giving cln-plugin=0.1.2 is bumped in #160, and this PR builds on top of that.

This will need rebasing once #160 gets merged, but given it was double at this stage and didn't require much more I went ahead and fixed it.

@sr-gi
Copy link
Member Author

sr-gi commented Jan 10, 2023

Rebased and amended this so it pulls from master.

I've also moved a bit more data to constants.rs to reduce the hardcoded stuff in main.rs.

Options include helper functions to convert them to their proper type after
`cln-plugin=0.1.2`. Use that to reduce the option parsing boilerplate.

Also move all names, descriptions and default values for options, rpc_methods and hooks
to a new file.
@sr-gi sr-gi force-pushed the cln-simplify-options branch from ca8b8ed to 6920c9b Compare January 10, 2023 14:27
@sr-gi
Copy link
Member Author

sr-gi commented Jan 10, 2023

Rebased and amended this so it pulls from master.

I've also moved a bit more data to constants.rs to reduce the hardcoded stuff in main.rs.

Looks like I've broken the plugin by doing this 😅, not exactly sure why though.

`auto-retry-delay` was set to be u16, but its default value was beyond u16::MAX.
@sr-gi
Copy link
Member Author

sr-gi commented Jan 11, 2023

Rebased and amended this so it pulls from master.
I've also moved a bit more data to constants.rs to reduce the hardcoded stuff in main.rs.

Looks like I've broken the plugin by doing this 😅, not exactly sure why though.

Fixed it, auto-retry-delay default value was beyond u16::MAX. Looks like this was not caught in 07caee2 because the default was set in two different places on the codebase:

)).option(ConfigOption::new(
"watchtower-auto-retry-delay",
Value::Integer(86400),
"the time (in seconds) that a retrier will wait before auto-retrying a failed tower. Defaults to once a day",

if let Value::Integer(x) = midstate.option("watchtower-auto-retry-delay").unwrap() {
x as u16
} else {
// We will never end up here, but we need to define an else. Should be fixed alongside the previous fixme.
3600

@sr-gi sr-gi merged commit 2a57a38 into talaia-labs:master Jan 11, 2023
@mariocynicys
Copy link
Collaborator

mariocynicys commented Jan 11, 2023

@sr-gi
One of the failures though wasn't in the e2e tests: https://github.com/talaia-labs/rust-teos/actions/runs/3884258978/jobs/6626583255

This panicked while checking if the retrier is idle. Guess the sleep time is too tight?

@sr-gi
Copy link
Member Author

sr-gi commented Jan 11, 2023

@sr-gi One of the failures though wasn't in the e2e tests: https://github.com/talaia-labs/rust-teos/actions/runs/3884258978/jobs/6626583255

This panicked while checking if the retrier is idle. Guess the sleep time is too tight?

That's my guess, unfortunately, I haven't been able to reproduce it locally

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cln-plugin Stuff related to watchtower-plugin easy to review easypeasy Seeking Code Review review me pls
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants