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

mutiny-server #1084

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

mutiny-server #1084

wants to merge 6 commits into from

Conversation

elnosh
Copy link
Contributor

@elnosh elnosh commented Mar 26, 2024

this is a wip of mutiny-server

There are other endpoints I will need to add but I have basic ones working right now (open channel, create invoice, pay invoice, send to address)

I would appreciate any reviews of what I've done so far :)

Copy link
Collaborator

@benthecarman benthecarman left a comment

Choose a reason for hiding this comment

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

looking good so far!

mutiny-server/src/routes.rs Outdated Show resolved Hide resolved
mutiny-server/src/sled.rs Outdated Show resolved Hide resolved
mutiny-server/src/main.rs Outdated Show resolved Hide resolved
mutiny-server/src/extractor.rs Show resolved Hide resolved
mutiny-core/src/lib.rs Outdated Show resolved Hide resolved
@benthecarman
Copy link
Collaborator

also looks like you need to run cargo fmt

@elnosh elnosh force-pushed the mutiny-server branch 2 times, most recently from f0fb430 to c3b3d62 Compare March 27, 2024 03:45
@elnosh
Copy link
Contributor Author

elnosh commented Mar 27, 2024

Added rest of endpoints. Need to fix this cargo doc failing

@elnosh elnosh force-pushed the mutiny-server branch 2 times, most recently from 204e7d2 to a40f067 Compare March 27, 2024 20:14
@elnosh elnosh marked this pull request as ready for review March 27, 2024 20:31
@elnosh elnosh changed the title (wip) mutiny server mutiny-sserver Mar 27, 2024
@elnosh elnosh changed the title mutiny-sserver mutiny-server Mar 27, 2024
@elnosh elnosh force-pushed the mutiny-server branch 2 times, most recently from eb9e540 to 178cb22 Compare March 28, 2024 21:04
Copy link
Collaborator

@benthecarman benthecarman left a comment

Choose a reason for hiding this comment

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

Mostly looks good to me just some style things.

I think we will need to properly figure out the issue you had with the docs CI. When I try to run cargo check in the root of mutiny-node now I get issues. Probably will need to figure out correct ways to handle the per-package-target stuff

Comment on lines 284 to 292
let fees = match invoice.fees_paid {
Some(fee) => fee,
None => {
return Err((
StatusCode::INTERNAL_SERVER_ERROR,
Json(json!({"error": "unable to pay invoice"})),
))
}
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

You do this style a bunch, it is cleaner to do let fees = invoice.fees_paid.ok_or((StatusCode::INTERNAL_SERVER_ERROR, Json(json!({"error": "unable to pay invoice"})))?

Comment on lines 461 to 462
true => "usable",
false => "unusable",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you make this into an enum?

@elnosh elnosh force-pushed the mutiny-server branch 3 times, most recently from 0ef9077 to 9a99b93 Compare April 4, 2024 17:44
None => None,
};

let pr = invoice.bolt11.clone().map(|bolt11| bolt11.to_string());
Copy link
Collaborator

Choose a reason for hiding this comment

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

you don't need this clone, can do as_ref() instead

Some(bolt11) => Some(bolt11.to_string()),
None => None,
};
let pr = invoice.bolt11.clone().map(|bolt11| bolt11.to_string());
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here

@elnosh elnosh force-pushed the mutiny-server branch 3 times, most recently from 21e13b1 to 6545820 Compare April 5, 2024 22:28
@elnosh elnosh requested a review from benthecarman April 6, 2024 15:44
@elnosh elnosh force-pushed the mutiny-server branch 2 times, most recently from 55fec7e to b6fece9 Compare April 11, 2024 13:27
Copy link
Collaborator

@benthecarman benthecarman left a comment

Choose a reason for hiding this comment

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

couple nits

Can you also add some commands to the just file? Make sure the clippy commands will run clippy for mutiny-server. Also would be good to add a somehting like just run-signet that will run mutiny server on signet with all the required params

@@ -0,0 +1,29 @@
[package]
name = "mutiny-server"
version = "0.3.7"
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you bump to 0.6.4

let config: Config = Config::parse();

let network = config.network();
let storage = SledStorage::new(&config.db_file, config.clone().password)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

can instead do config.password.clone() here, that way you don't need to clone the entire config

Comment on lines +433 to +427
let state = match channel.is_usable {
true => ChannelState::Usable,
false => ChannelState::Unusable,
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of all this, can just have ChannelState impl Serialize, maybe just need to add the serde flag for all lowercase

@elnosh elnosh force-pushed the mutiny-server branch 2 times, most recently from a1dfbd6 to 8dc8981 Compare April 12, 2024 19:05
@TonyGiorgio
Copy link
Contributor

Would like to figure out how to get the workspaces to work correctly. I can look into that more. Skimmed the structure and looks pretty sound. Will play with it more soon and can merge whenever the base looks good, can then can keep iterating on the commands. Nice work so far!

@TonyGiorgio
Copy link
Contributor

Actually, I think we should use rocksdb instead of sleddb.

@elnosh elnosh force-pushed the mutiny-server branch 2 times, most recently from 6e46c9f to 0bfcdf8 Compare April 19, 2024 20:10
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.

3 participants