PoC removing cmake and protoc as system-wide dependencies#669
PoC removing cmake and protoc as system-wide dependencies#669sergey3bv wants to merge 2 commits intoMostroP2P:mainfrom
Conversation
WalkthroughThis PR replaces system-wide protoc installation with a vendored protoc wrapper. It removes protobuf compilation dependencies from build scripts, Dockerfiles, and installation documentation while adding the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.cargo/protoc-wrapper:
- Around line 27-44: The script currently only tests file mode with -x for the
PROTOC_BIN candidate which can accept wrong-architecture binaries; change the
validation logic that sets PROTOC_BIN (the find branch and the command -v
branch) to actually run the candidate (e.g., invoke "$PROTOC_BIN" --version or
similar) and verify it returns a successful exit status before accepting it, and
only then proceed to exec "$PROTOC_BIN" "$@"; update error messages to reflect a
failed execution check for clarity.
In `@README.md`:
- Around line 828-829: Update the Ubuntu installation line that currently only
installs build-essential to include the other native-build dependencies listed
elsewhere in the README: add libsqlite3-dev and pkg-config (so the apt install
command installs build-essential, libsqlite3-dev, and pkg-config) and ensure the
comment about macOS remains unchanged; update the same code block/snippet where
the current sudo apt install build-essential line appears so both sections are
consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 25e94ec0-8c63-4aad-b066-669fde6def54
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
.cargo/config.toml.cargo/protoc-wrapperCargo.tomlCross.tomlINSTALL.mdREADME.mdbuild.rsdocker/Dockerfiledocker/dockerfile-startos
💤 Files with no reviewable changes (1)
- Cross.toml
.cargo/protoc-wrapper
Outdated
| if [ -z "$PROTOC_BIN" ] || [ ! -x "$PROTOC_BIN" ]; then | ||
| PROTOC_BIN=$(find "$CARGO_HOME/registry/src" \ | ||
| -name "protoc" \ | ||
| -path "*/protoc-bin-vendored-*/bin/protoc" \ | ||
| 2>/dev/null | sort | tail -1) | ||
| fi | ||
|
|
||
| if [ -z "$PROTOC_BIN" ] || [ ! -x "$PROTOC_BIN" ]; then | ||
| # Vendored binary not found — fall back to whatever protoc is on PATH. | ||
| PROTOC_BIN=$(command -v protoc 2>/dev/null) | ||
| if [ -z "$PROTOC_BIN" ]; then | ||
| echo "protoc-wrapper: vendored protoc not found under $CARGO_HOME and no system protoc on PATH." >&2 | ||
| echo "Run 'cargo fetch' first, or install protoc (brew install protobuf / apt install protobuf-compiler)." >&2 | ||
| exit 1 | ||
| fi | ||
| fi | ||
|
|
||
| exec "$PROTOC_BIN" "$@" |
There was a problem hiding this comment.
Validate candidate protoc by execution, not just permission bits.
On Line 27/Line 34, -x only checks mode. A wrong-arch binary can still pass and then fail at Line 44 with exec format error.
🛠️ Suggested hardening
if [ -z "$PROTOC_BIN" ] || [ ! -x "$PROTOC_BIN" ]; then
PROTOC_BIN=$(find "$CARGO_HOME/registry/src" \
-name "protoc" \
-path "*/protoc-bin-vendored-*/bin/protoc" \
2>/dev/null | sort | tail -1)
fi
-if [ -z "$PROTOC_BIN" ] || [ ! -x "$PROTOC_BIN" ]; then
+# Reject non-runnable candidates (e.g. wrong architecture) before PATH fallback.
+if [ -n "$PROTOC_BIN" ] && ! "$PROTOC_BIN" --version >/dev/null 2>&1; then
+ PROTOC_BIN=""
+fi
+
+if [ -z "$PROTOC_BIN" ] || [ ! -x "$PROTOC_BIN" ]; then
# Vendored binary not found — fall back to whatever protoc is on PATH.
PROTOC_BIN=$(command -v protoc 2>/dev/null)
if [ -z "$PROTOC_BIN" ]; then
echo "protoc-wrapper: vendored protoc not found under $CARGO_HOME and no system protoc on PATH." >&2
echo "Run 'cargo fetch' first, or install protoc (brew install protobuf / apt install protobuf-compiler)." >&2
exit 1
fi
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [ -z "$PROTOC_BIN" ] || [ ! -x "$PROTOC_BIN" ]; then | |
| PROTOC_BIN=$(find "$CARGO_HOME/registry/src" \ | |
| -name "protoc" \ | |
| -path "*/protoc-bin-vendored-*/bin/protoc" \ | |
| 2>/dev/null | sort | tail -1) | |
| fi | |
| if [ -z "$PROTOC_BIN" ] || [ ! -x "$PROTOC_BIN" ]; then | |
| # Vendored binary not found — fall back to whatever protoc is on PATH. | |
| PROTOC_BIN=$(command -v protoc 2>/dev/null) | |
| if [ -z "$PROTOC_BIN" ]; then | |
| echo "protoc-wrapper: vendored protoc not found under $CARGO_HOME and no system protoc on PATH." >&2 | |
| echo "Run 'cargo fetch' first, or install protoc (brew install protobuf / apt install protobuf-compiler)." >&2 | |
| exit 1 | |
| fi | |
| fi | |
| exec "$PROTOC_BIN" "$@" | |
| if [ -z "$PROTOC_BIN" ] || [ ! -x "$PROTOC_BIN" ]; then | |
| PROTOC_BIN=$(find "$CARGO_HOME/registry/src" \ | |
| -name "protoc" \ | |
| -path "*/protoc-bin-vendored-*/bin/protoc" \ | |
| 2>/dev/null | sort | tail -1) | |
| fi | |
| # Reject non-runnable candidates (e.g. wrong architecture) before PATH fallback. | |
| if [ -n "$PROTOC_BIN" ] && ! "$PROTOC_BIN" --version >/dev/null 2>&1; then | |
| PROTOC_BIN="" | |
| fi | |
| if [ -z "$PROTOC_BIN" ] || [ ! -x "$PROTOC_BIN" ]; then | |
| # Vendored binary not found — fall back to whatever protoc is on PATH. | |
| PROTOC_BIN=$(command -v protoc 2>/dev/null) | |
| if [ -z "$PROTOC_BIN" ]; then | |
| echo "protoc-wrapper: vendored protoc not found under $CARGO_HOME and no system protoc on PATH." >&2 | |
| echo "Run 'cargo fetch' first, or install protoc (brew install protobuf / apt install protobuf-compiler)." >&2 | |
| exit 1 | |
| fi | |
| fi | |
| exec "$PROTOC_BIN" "$@" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.cargo/protoc-wrapper around lines 27 - 44, The script currently only tests
file mode with -x for the PROTOC_BIN candidate which can accept
wrong-architecture binaries; change the validation logic that sets PROTOC_BIN
(the find branch and the command -v branch) to actually run the candidate (e.g.,
invoke "$PROTOC_BIN" --version or similar) and verify it returns a successful
exit status before accepting it, and only then proceed to exec "$PROTOC_BIN"
"$@"; update error messages to reflect a failed execution check for clarity.
| sudo apt install build-essential # Ubuntu | ||
| # macOS: Xcode Command Line Tools provide the C compiler (xcode-select --install) |
There was a problem hiding this comment.
Development dependency command is currently incomplete for Ubuntu.
Line 828 only installs build-essential, but this README also lists libsqlite3-dev and pkg-config as required for native builds. Please keep these sections consistent.
📄 Suggested doc fix
- sudo apt install build-essential # Ubuntu
- # macOS: Xcode Command Line Tools provide the C compiler (xcode-select --install)
+ sudo apt install -y build-essential libsqlite3-dev pkg-config sqlite3 # Ubuntu/Debian
+ # macOS: xcode-select --install && brew install pkg-config sqlite3🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 828 - 829, Update the Ubuntu installation line that
currently only installs build-essential to include the other native-build
dependencies listed elsewhere in the README: add libsqlite3-dev and pkg-config
(so the apt install command installs build-essential, libsqlite3-dev, and
pkg-config) and ensure the comment about macOS remains unchanged; update the
same code block/snippet where the current sudo apt install build-essential line
appears so both sections are consistent.
|
This is nice imo, let me understand what is the advantage...we don't need anymore to have protoc downloaded in github actions and also compiled during build, but we leverage a vendored versions? |
The main benefit is self-contained toolchain. Removing third-party at dependencies OS level leads to more reproducible and simple builds, as Cargo manages all of them by itself. As I see it, ideally the project may reach a point where setup is simple |
Crystal clear! One question, did this build also on windows machines, i am in office and I am testing build, but windows is not happy. Could you check this? |
Sorry, there is no any Windows machine in sight. My guess is that solution is tied to shell script. I will try to come up with a more agnostic solution |
I know that |
|
@arkanoider, unfortunately, I was not able to come up with a clean solution for this. I dumped everything to the branch so a rust guru can continue from here. As I said this PR is a proof of concept rather than a proper solution. |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
vendor/fedimint-tonic-lnd/examples/cancel_invoice.rs (1)
18-27: Keep file arguments as native path values.
get_info.rsandget_version.rsalready passcert_fileandmacaroon_filethrough as native values, but this example forces both through UTF-8 on Lines 18-27. Leaving the path arguments in their native form avoids an extra decoding failure mode and keeps the examples consistent.Suggested fix
- let cert_file: String = args + let cert_file = args .next() .expect("missing arguments: cert file, macaroon file, payment hash") - .into_string() - .expect("cert_file is not UTF-8"); - let macaroon_file: String = args + ; + let macaroon_file = args .next() .expect("missing argument: macaroon file, payment hash") - .into_string() - .expect("macaroon_file is not UTF-8"); + ;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vendor/fedimint-tonic-lnd/examples/cancel_invoice.rs` around lines 18 - 27, The example forces CLI path args through UTF-8 (into_string()) for cert_file and macaroon_file; change them to preserve native paths like the other examples (get_info.rs/get_version.rs) by using into_os_string() and storing them as OsString (cert_file, macaroon_file) instead of String, and keep the same expect() argument-missing messages but remove the “is not UTF-8” checks since no UTF-8 conversion will occur.vendor/fedimint-tonic-lnd/src/lib.rs (1)
273-275: Unnecessary.clone()on final client initialization.The last
svc.clone()anduri.clone()calls (for thestaterpcclient) are unnecessary sincesvcanduriare not used afterward. This is a minor optimization.Proposed fix (if staterpc is the last feature)
#[cfg(feature = "staterpc")] - state: staterpc::state_client::StateClient::with_origin(svc.clone(), uri.clone()), + state: staterpc::state_client::StateClient::with_origin(svc, uri),Note: This only applies if
staterpcis guaranteed to be the last field. Given the feature-gated nature of these fields, the current approach with consistent.clone()is safer and more maintainable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vendor/fedimint-tonic-lnd/src/lib.rs` around lines 273 - 275, The final staterpc client initialization uses unnecessary .clone() calls on svc and uri (state: staterpc::state_client::StateClient::with_origin(svc.clone(), uri.clone())), so remove the clones and pass svc and uri by value when staterpc is the last field; update the expression to StateClient::with_origin(svc, uri) ensuring svc and uri are not used after this point and compile errors are resolved by adjusting ownership if needed.vendor/fedimint-tonic-lnd/examples/subscribe_invoices.rs (1)
13-20: Inconsistent argument handling compared to other examples.The
addressis converted toStringon line 20, butcert_fileandmacaroon_fileremain asOsString. While this works (sinceconnect()acceptsimpl AsRef<Path>), it's inconsistent withintercept_htlcs.rsandtrack_payment.rswhich convert all arguments toString. Consider aligning the pattern for consistency across examples.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vendor/fedimint-tonic-lnd/examples/subscribe_invoices.rs` around lines 13 - 20, The args handling is inconsistent: only address is converted to String while cert_file and macaroon_file remain OsString; update the argument parsing so cert_file and macaroon_file are also converted to String (using their .into_string().expect(...) calls) to match address and align with intercept_htlcs.rs/track_payment.rs patterns, keeping clear expect messages and leaving the rest of the code (e.g., connect()) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@vendor/fedimint-tonic-lnd/build.rs`:
- Around line 3-10: The build script fails to tell Cargo to re-run when the
PROTOC env var changes; inside main() (before using std::env::var_os("PROTOC")
and alongside the existing println!("cargo:rerun-if-env-changed=LND_REPO_DIR")),
emit a cargo directive for PROTOC by printing cargo:rerun-if-env-changed=PROTOC
so Cargo will re-run the build script when PROTOC changes.
In `@vendor/fedimint-tonic-lnd/examples/get_info.rs`:
- Around line 9-11: Add a fallback entrypoint guarded with #[cfg(not(feature =
"lightningrpc"))] so the example compiles when the feature is disabled: create a
non-async main (or #[tokio::main] if async behavior is required) with the same
name main that prints a clear message instructing the user to enable the
"lightningrpc" feature (and optionally returns a non-zero exit code) so building
the example fails with a helpful hint instead of a compiler error; update or
keep the existing #[cfg(feature = "lightningrpc")] async fn main() as-is.
In `@vendor/fedimint-tonic-lnd/examples/intercept_htlcs.rs`:
- Line 59: Fix the typo in the inline comment near the action field in
intercept_htlcs.rs: change "Resume fordwarding of intercepted HTLC" to "Resume
forwarding of intercepted HTLC" so the comment for the action: 2 entry reads
correctly; update the comment text adjacent to the action field to use
"forwarding".
- Line 43: The panic message is a copy-paste mistake: replace the incorrect
"Failed to call subscribe_invoices" message on the .expect(...) following the
htlc_interceptor call with a correct, descriptive message (e.g., "Failed to call
htlc_interceptor" or "Failed to subscribe to HTLC interceptor") so the .expect
on the htlc_interceptor invocation accurately reflects the failing operation;
locate the .expect(...) attached to the htlc_interceptor call in
intercept_htlcs.rs and update the string accordingly.
- Around line 8-9: Move the feature-gate attribute so it applies to the whole
async main function: place #[cfg(feature = "routerrpc")] before the proc-macro
attribute #[tokio::main] (i.e., swap the two attributes above the main function)
so the cfg is evaluated prior to the tokio::main expansion; update the
attributes above the `main` function in intercept_htlcs.rs accordingly.
In `@vendor/fedimint-tonic-lnd/examples/subscribe_invoices.rs`:
- Around line 8-9: The attributes on the main function are ordered incorrectly;
move the feature-gate attribute so it precedes the tokio runtime attribute by
swapping #[cfg(feature = "lightningrpc")] to be above #[tokio::main] in
subscribe_invoices.rs (same fix as applied in intercept_htlcs.rs) so the
conditional compilation is evaluated before applying #[tokio::main].
In `@vendor/fedimint-tonic-lnd/examples/track_payment.rs`:
- Around line 8-9: The attributes on the async main in track_payment.rs are
ordered incorrectly; move the feature gate attribute so #[cfg(feature =
"routerrpc")] appears before #[tokio::main] to ensure the cfg is applied to the
whole item (the async runtime attribute), i.e., swap the two attributes that
decorate the main function so the cfg comes first.
In `@vendor/fedimint-tonic-lnd/src/lib.rs`:
- Around line 330-332: The code currently unwraps CryptoProvider::get_default()
and panics; change this to return a proper error when no default provider is
installed: update the surrounding function to return Result<..., Error> (or use
an existing error type), replace the expect call on
CryptoProvider::get_default().clone() with an if let / match that maps None to
an Err(e.g., a descriptive error like "no default crypto provider installed")
and Some(provider) to continue, and propagate that error upward instead of
panicking; reference CryptoProvider::get_default() and the local provider
binding when making the change.
---
Nitpick comments:
In `@vendor/fedimint-tonic-lnd/examples/cancel_invoice.rs`:
- Around line 18-27: The example forces CLI path args through UTF-8
(into_string()) for cert_file and macaroon_file; change them to preserve native
paths like the other examples (get_info.rs/get_version.rs) by using
into_os_string() and storing them as OsString (cert_file, macaroon_file) instead
of String, and keep the same expect() argument-missing messages but remove the
“is not UTF-8” checks since no UTF-8 conversion will occur.
In `@vendor/fedimint-tonic-lnd/examples/subscribe_invoices.rs`:
- Around line 13-20: The args handling is inconsistent: only address is
converted to String while cert_file and macaroon_file remain OsString; update
the argument parsing so cert_file and macaroon_file are also converted to String
(using their .into_string().expect(...) calls) to match address and align with
intercept_htlcs.rs/track_payment.rs patterns, keeping clear expect messages and
leaving the rest of the code (e.g., connect()) unchanged.
In `@vendor/fedimint-tonic-lnd/src/lib.rs`:
- Around line 273-275: The final staterpc client initialization uses unnecessary
.clone() calls on svc and uri (state:
staterpc::state_client::StateClient::with_origin(svc.clone(), uri.clone())), so
remove the clones and pass svc and uri by value when staterpc is the last field;
update the expression to StateClient::with_origin(svc, uri) ensuring svc and uri
are not used after this point and compile errors are resolved by adjusting
ownership if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0be15e73-1f87-4245-a5d7-0b9cedd8a5f3
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockvendor/fedimint-tonic-lnd/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
.cargo/config.tomlCargo.tomlbuild.rsvendor/fedimint-tonic-lnd/.cargo-okvendor/fedimint-tonic-lnd/.cargo_vcs_info.jsonvendor/fedimint-tonic-lnd/.github/workflows/rust.ymlvendor/fedimint-tonic-lnd/.gitignorevendor/fedimint-tonic-lnd/Cargo.tomlvendor/fedimint-tonic-lnd/Cargo.toml.origvendor/fedimint-tonic-lnd/README.mdvendor/fedimint-tonic-lnd/build.rsvendor/fedimint-tonic-lnd/examples/cancel_invoice.rsvendor/fedimint-tonic-lnd/examples/get_info.rsvendor/fedimint-tonic-lnd/examples/get_version.rsvendor/fedimint-tonic-lnd/examples/intercept_htlcs.rsvendor/fedimint-tonic-lnd/examples/subscribe_invoices.rsvendor/fedimint-tonic-lnd/examples/track_payment.rsvendor/fedimint-tonic-lnd/src/error.rsvendor/fedimint-tonic-lnd/src/lib.rsvendor/fedimint-tonic-lnd/vendor/invoicesrpc/invoices.protovendor/fedimint-tonic-lnd/vendor/lightning.protovendor/fedimint-tonic-lnd/vendor/peersrpc/peers.protovendor/fedimint-tonic-lnd/vendor/routerrpc/router.protovendor/fedimint-tonic-lnd/vendor/signrpc/signer.protovendor/fedimint-tonic-lnd/vendor/staterpc/state.protovendor/fedimint-tonic-lnd/vendor/verrpc/verrpc.protovendor/fedimint-tonic-lnd/vendor/walletrpc/walletkit.proto
✅ Files skipped from review due to trivial changes (8)
- vendor/fedimint-tonic-lnd/.gitignore
- vendor/fedimint-tonic-lnd/.cargo_vcs_info.json
- vendor/fedimint-tonic-lnd/.cargo-ok
- vendor/fedimint-tonic-lnd/.github/workflows/rust.yml
- vendor/fedimint-tonic-lnd/README.md
- vendor/fedimint-tonic-lnd/vendor/verrpc/verrpc.proto
- vendor/fedimint-tonic-lnd/Cargo.toml
- vendor/fedimint-tonic-lnd/Cargo.toml.orig
🚧 Files skipped from review as they are similar to previous changes (2)
- build.rs
- Cargo.toml
| fn main() -> std::io::Result<()> { | ||
| if std::env::var_os("PROTOC").is_none() { | ||
| let protoc = protoc_bin_vendored::protoc_bin_path() | ||
| .expect("protoc-bin-vendored: could not determine protoc path"); | ||
| std::env::set_var("PROTOC", protoc); | ||
| } | ||
|
|
||
| println!("cargo:rerun-if-env-changed=LND_REPO_DIR"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "build.rs" -path "*/fedimint-tonic-lnd/*" | head -5Repository: MostroP2P/mostro
Length of output: 96
🏁 Script executed:
cat -n vendor/fedimint-tonic-lnd/build.rs 2>/dev/null || find . -name "build.rs" -path "*fedimint-tonic-lnd*" -exec cat -n {} \;Repository: MostroP2P/mostro
Length of output: 1626
🏁 Script executed:
# Check if file exists and get its size
wc -l vendor/fedimint-tonic-lnd/build.rs 2>/dev/null || echo "File not found in expected location, searching..."Repository: MostroP2P/mostro
Length of output: 97
🏁 Script executed:
# Search for fedimint-tonic-lnd in the repo
find . -type d -name "fedimint-tonic-lnd" | head -3Repository: MostroP2P/mostro
Length of output: 87
Track PROTOC in Cargo's rebuild fingerprint.
This script conditionally uses either a system or vendored protoc based on the PROTOC environment variable (lines 4–8), but never declares that dependency to Cargo. When PROTOC changes between builds, Cargo has no reason to re-run the build script, potentially leaving stale bindings in place. Add cargo:rerun-if-env-changed=PROTOC at the start of main().
Suggested fix
fn main() -> std::io::Result<()> {
+ println!("cargo:rerun-if-env-changed=PROTOC");
if std::env::var_os("PROTOC").is_none() {
let protoc = protoc_bin_vendored::protoc_bin_path()
.expect("protoc-bin-vendored: could not determine protoc path");
std::env::set_var("PROTOC", protoc);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vendor/fedimint-tonic-lnd/build.rs` around lines 3 - 10, The build script
fails to tell Cargo to re-run when the PROTOC env var changes; inside main()
(before using std::env::var_os("PROTOC") and alongside the existing
println!("cargo:rerun-if-env-changed=LND_REPO_DIR")), emit a cargo directive for
PROTOC by printing cargo:rerun-if-env-changed=PROTOC so Cargo will re-run the
build script when PROTOC changes.
| #[tokio::main] | ||
| #[cfg(feature = "lightningrpc")] | ||
| async fn main() { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check if the file exists and view its contents
if [ -f "vendor/fedimint-tonic-lnd/examples/get_info.rs" ]; then
echo "=== File exists, showing contents ==="
cat -n "vendor/fedimint-tonic-lnd/examples/get_info.rs"
else
echo "File not found at vendor/fedimint-tonic-lnd/examples/get_info.rs"
# Try to find similar files
echo "=== Searching for get_info.rs ==="
find . -name "get_info.rs" -type f 2>/dev/null
fiRepository: MostroP2P/mostro
Length of output: 1833
🏁 Script executed:
#!/bin/bash
# Check what sibling examples exist in the same directory
if [ -d "vendor/fedimint-tonic-lnd/examples" ]; then
echo "=== Sibling examples in vendor/fedimint-tonic-lnd/examples ==="
ls -la vendor/fedimint-tonic-lnd/examples/
echo ""
echo "=== Checking main functions in sibling examples ==="
for file in vendor/fedimint-tonic-lnd/examples/*.rs; do
if [ -f "$file" ]; then
echo "--- $(basename $file) ---"
grep -n "fn main" "$file" | head -20
echo ""
fi
done
fiRepository: MostroP2P/mostro
Length of output: 967
🏁 Script executed:
#!/bin/bash
# Check the actual content of a few sibling examples to see if they have fallback main
echo "=== Checking cancel_invoice.rs for fallback main ==="
head -20 "vendor/fedimint-tonic-lnd/examples/cancel_invoice.rs"
echo ""
echo "=== Checking get_version.rs for fallback main ==="
head -20 "vendor/fedimint-tonic-lnd/examples/get_version.rs"
echo ""
echo "=== Checking intercept_htlcs.rs for fallback main ==="
head -20 "vendor/fedimint-tonic-lnd/examples/intercept_htlcs.rs"Repository: MostroP2P/mostro
Length of output: 2684
🏁 Script executed:
#!/bin/bash
# Search for cfg(not(feature patterns in all sibling examples
echo "=== Searching for fallback cfg(not patterns in all examples ==="
grep -n "cfg(not(feature" vendor/fedimint-tonic-lnd/examples/*.rs || echo "No fallback cfg(not patterns found"
echo ""
# Also check the full content of a couple more examples
echo "=== Full content of subscribe_invoices.rs ==="
cat "vendor/fedimint-tonic-lnd/examples/subscribe_invoices.rs"Repository: MostroP2P/mostro
Length of output: 2078
Add a fallback main for the missing feature.
With lightningrpc disabled, this file has no main, so cargo build --example get_info fails with an opaque compiler error instead of telling the user which feature to enable. A #[cfg(not(feature = "lightningrpc"))] entrypoint makes the example self-describing.
Suggested fix
+#[cfg(not(feature = "lightningrpc"))]
+fn main() {
+ eprintln!("enable the `lightningrpc` feature to run this example");
+ std::process::exit(1);
+}
+
#[tokio::main]
#[cfg(feature = "lightningrpc")]
async fn main() {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[tokio::main] | |
| #[cfg(feature = "lightningrpc")] | |
| async fn main() { | |
| #[cfg(not(feature = "lightningrpc"))] | |
| fn main() { | |
| eprintln!("enable the `lightningrpc` feature to run this example"); | |
| std::process::exit(1); | |
| } | |
| #[tokio::main] | |
| #[cfg(feature = "lightningrpc")] | |
| async fn main() { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vendor/fedimint-tonic-lnd/examples/get_info.rs` around lines 9 - 11, Add a
fallback entrypoint guarded with #[cfg(not(feature = "lightningrpc"))] so the
example compiles when the feature is disabled: create a non-async main (or
#[tokio::main] if async behavior is required) with the same name main that
prints a clear message instructing the user to enable the "lightningrpc" feature
(and optionally returns a non-zero exit code) so building the example fails with
a helpful hint instead of a compiler error; update or keep the existing
#[cfg(feature = "lightningrpc")] async fn main() as-is.
| #[tokio::main] | ||
| #[cfg(feature = "routerrpc")] |
There was a problem hiding this comment.
Incorrect attribute ordering: #[cfg] should precede #[tokio::main].
The #[cfg(feature = "routerrpc")] attribute should be placed before #[tokio::main] to ensure the feature gate applies to the entire function including the proc-macro expansion.
Proposed fix
-#[tokio::main]
#[cfg(feature = "routerrpc")]
+#[tokio::main]
async fn main() {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[tokio::main] | |
| #[cfg(feature = "routerrpc")] | |
| #[cfg(feature = "routerrpc")] | |
| #[tokio::main] | |
| async fn main() { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vendor/fedimint-tonic-lnd/examples/intercept_htlcs.rs` around lines 8 - 9,
Move the feature-gate attribute so it applies to the whole async main function:
place #[cfg(feature = "routerrpc")] before the proc-macro attribute
#[tokio::main] (i.e., swap the two attributes above the main function) so the
cfg is evaluated prior to the tokio::main expansion; update the attributes above
the `main` function in intercept_htlcs.rs accordingly.
| .router() | ||
| .htlc_interceptor(stream) | ||
| .await | ||
| .expect("Failed to call subscribe_invoices") |
There was a problem hiding this comment.
Copy-paste error in panic message.
The error message references "subscribe_invoices" but this is htlc_interceptor.
Proposed fix
- .expect("Failed to call subscribe_invoices")
+ .expect("Failed to call htlc_interceptor")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .expect("Failed to call subscribe_invoices") | |
| .expect("Failed to call htlc_interceptor") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vendor/fedimint-tonic-lnd/examples/intercept_htlcs.rs` at line 43, The panic
message is a copy-paste mistake: replace the incorrect "Failed to call
subscribe_invoices" message on the .expect(...) following the htlc_interceptor
call with a correct, descriptive message (e.g., "Failed to call
htlc_interceptor" or "Failed to subscribe to HTLC interceptor") so the .expect
on the htlc_interceptor invocation accurately reflects the failing operation;
locate the .expect(...) attached to the htlc_interceptor call in
intercept_htlcs.rs and update the string accordingly.
|
|
||
| let response = fedimint_tonic_lnd::routerrpc::ForwardHtlcInterceptResponse { | ||
| incoming_circuit_key: htlc.incoming_circuit_key, | ||
| action: 2, // Resume fordwarding of intercepted HTLC |
There was a problem hiding this comment.
Typo: "fordwarding" → "forwarding".
Proposed fix
- action: 2, // Resume fordwarding of intercepted HTLC
+ action: 2, // Resume forwarding of intercepted HTLC📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| action: 2, // Resume fordwarding of intercepted HTLC | |
| action: 2, // Resume forwarding of intercepted HTLC |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vendor/fedimint-tonic-lnd/examples/intercept_htlcs.rs` at line 59, Fix the
typo in the inline comment near the action field in intercept_htlcs.rs: change
"Resume fordwarding of intercepted HTLC" to "Resume forwarding of intercepted
HTLC" so the comment for the action: 2 entry reads correctly; update the comment
text adjacent to the action field to use "forwarding".
| #[tokio::main] | ||
| #[cfg(feature = "lightningrpc")] |
There was a problem hiding this comment.
Incorrect attribute ordering: #[cfg] should precede #[tokio::main].
Same issue as in intercept_htlcs.rs — the feature gate should come first.
Proposed fix
-#[tokio::main]
#[cfg(feature = "lightningrpc")]
+#[tokio::main]
async fn main() {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[tokio::main] | |
| #[cfg(feature = "lightningrpc")] | |
| #[cfg(feature = "lightningrpc")] | |
| #[tokio::main] | |
| async fn main() { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vendor/fedimint-tonic-lnd/examples/subscribe_invoices.rs` around lines 8 - 9,
The attributes on the main function are ordered incorrectly; move the
feature-gate attribute so it precedes the tokio runtime attribute by swapping
#[cfg(feature = "lightningrpc")] to be above #[tokio::main] in
subscribe_invoices.rs (same fix as applied in intercept_htlcs.rs) so the
conditional compilation is evaluated before applying #[tokio::main].
| #[tokio::main] | ||
| #[cfg(feature = "routerrpc")] |
There was a problem hiding this comment.
Incorrect attribute ordering: #[cfg] should precede #[tokio::main].
Same issue as other examples — the feature gate should come first.
Proposed fix
-#[tokio::main]
#[cfg(feature = "routerrpc")]
+#[tokio::main]
async fn main() {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[tokio::main] | |
| #[cfg(feature = "routerrpc")] | |
| #[cfg(feature = "routerrpc")] | |
| #[tokio::main] | |
| async fn main() { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vendor/fedimint-tonic-lnd/examples/track_payment.rs` around lines 8 - 9, The
attributes on the async main in track_payment.rs are ordered incorrectly; move
the feature gate attribute so #[cfg(feature = "routerrpc")] appears before
#[tokio::main] to ensure the cfg is applied to the whole item (the async runtime
attribute), i.e., swap the two attributes that decorate the main function so the
cfg comes first.
| let provider = CryptoProvider::get_default() | ||
| .clone() | ||
| .expect("Must install default crypto provider"); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
rustls CryptoProvider get_default None behavior
💡 Result:
The rustls CryptoProvider::get_default function returns Option<&'static Arc> and explicitly returns None if no default CryptoProvider has been set for the process yet. A default must be installed first using CryptoProvider::install_default (e.g., rustls::crypto::aws_lc_rs::default_provider.install_default), typically early in main. This can only be called once per process. Libraries should rely on this process-level default via get_default, while ClientConfig::builder and ServerConfig::builder will panic if no default is available and crate features are ambiguous.
Citations:
- 1: https://docs.rs/rustls/latest/rustls/crypto/struct.CryptoProvider.html
- 2: Anyone seeing this? call CryptoProvider::install_default() rustls/tokio-rustls#110
- 3: https://docs.rs/rustls-ffi/latest/rustls_ffi/crypto_provider/fn.rustls_crypto_provider_builder_new_from_default.html
- 4: https://tg-rs.github.io/tgbot/rustls/crypto/struct.CryptoProvider.html
- 5: https://docs.rs/rustls/latest/rustls/
Panic if no crypto provider is installed.
CryptoProvider::get_default() returns None if no default crypto provider has been installed in the process, causing a panic here. While this aligns with rustls expectations (a default crypto provider must be installed early, typically via CryptoProvider::install_default()), the error message is already clear. Consider whether returning an error instead of panicking would be more appropriate for this library context.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vendor/fedimint-tonic-lnd/src/lib.rs` around lines 330 - 332, The code
currently unwraps CryptoProvider::get_default() and panics; change this to
return a proper error when no default provider is installed: update the
surrounding function to return Result<..., Error> (or use an existing error
type), replace the expect call on CryptoProvider::get_default().clone() with an
if let / match that maps None to an Err(e.g., a descriptive error like "no
default crypto provider installed") and Some(provider) to continue, and
propagate that error upward instead of panicking; reference
CryptoProvider::get_default() and the local provider binding when making the
change.
2 participants
I saw that the project relies on system-wide applications and decided to try bring them into Cargo. The result is working prototype which passes tests without protoc installed. This way number of moving parts should decrease making building and deploying easier. What do you think?
Summary by CodeRabbit
New Features
Documentation
Chores