Propagate error if the fs read for ohttp keys fails during config

[shinghim]

Fixes #398

[coveralls]

Pull Request Test Coverage Report for Build 12425617115

Details

• 12 of 21 (57.14%) changed or added relevant lines in 1 file are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage increased (+0.09%) to 67.137%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin-cli/src/app/config.rs	12	21	57.14%

Files with Coverage Reduction	New Missed Lines	%
payjoin-cli/src/app/config.rs	1	56.25%

Totals
Change from base Build 12331889117:	0.09%
Covered Lines:	3142
Relevant Lines:	4680

---

💛 - Coveralls

[DanGould]

How did you test this? I manually sent a GET request to payjo.in/ohttp-keys, placed the response ohttp-keys file in payjoin-cli, set up the following config:

pj_directory="https://payjo.in"
ohttp_relay="https://pj.bobspacebkk.com"
ohttp_keys="<payjoin-cli working directory from which the program is called>/ohttp-keys"

and ran cargo run --features v2 -- receive 66666 from within the payjoin-cli dir, only to get:

Error: Failed to parse config

Caused by:
    invalid type: string "/Users/dan/f/dev/payjoin/payjoin-cli/ohttp-keys", expected a sequence

Perhaps let config = AppConfig::new(&matches).with_context(|| "Failed to parse config")?; where this is called is not propagating Display correctly (I'd like to get rid of anyhow for proper typed errors and a simple Error = Box<std::error::Error + Send + Sync> wrapper but that's a bit out of context)

OR perhaps my input to the config.toml isn't parsed as a path to be passed to std::file::read

[shinghim]

@DanGould ah i wrongly assumed that since some of the tests failed when i incorrectly implemented this, that when they passed, it would mean this part is tested. Let me take a deeper look into this and get it tested. I'll update here once i figure it out

[shinghim]

@DanGould I used the example.cargo.toml file to test and got the same error in both master and this branch:

Error: Failed to parse config

Caused by:
    invalid type: string "./path/to/ohttp_keys", expected a sequence

I'm gonna mess around with it a bit - there's definitely something wrong with my implementation here

[shinghim]

Ok, I think I've figured it out. It looks like the error actually IS getting propagated already. I tested this by calling:

cargo run --features v2 -- receive 66666 --ohttp-keys /path/that/doesnt/exist

which resulted in:

[2024-12-12T14:39:18Z ERROR payjoin_cli::app::config] Failed to read ohttp_keys file: No such file or directory (os error 2)
Error: Failed to parse config

Caused by:
    invalid type: string "/Users/shingng/Desktop/test.txt", expected a sequence

so it looks like the error is being propagated since i can see this bit: Failed to read ohttp_keys file: in the output.

However, there's an issue when the CLI argument ohttp-keys isn't present, at which point it will fall back on reading the config file that was set as a source:

 pub(crate) fn new(matches: &ArgMatches) -> Result<Self, ConfigError> {
        let builder = Config::builder()
            .set_default("bitcoind_rpchost", "http://localhost:18443")?
           ...
            .add_source(File::new("config.toml", FileFormat::Toml).required(false)); // <--- here

This is a problem because the current code that parses the CLI:
matches.get_one::<String>("ohttp_keys")
doesn't consider the source that was added previously with .add_source() until the Config is built later via let config = builder.build()?;

From the .add_source() documentation:

Registers new Source in this builder.
Calling this method does not invoke any I/ O. Source is only saved in internal register for later use.

So, the issue isn't one of error propagation, but how to use the value in the config file as the file path for the ohttp keys. It looks like setting the value after the Config is built has been deprecated. Would it make sense to set the default path to be the current directory, and then the user can specify a an override by setting ohttp-keys?

[DanGould]

Kudos for keeping after it and seeing this investigation through. Super proud for you to have chosen to bless rust-payjoin with these contributions 🏎️

Would it make sense to set the default path to be the current directory, and then the user can specify a an override by setting ohttp-keys?

(Default directory for configuration and persistent data is related to #64.) The original intent with this design was for fetch_ohttp_keys to be called if no path to ohttp-keys OhttpKeys was set, and to fail if some path was set but was unable to be read as an explicit config. If we just set the default path to the current directory I'm concerned that we'd call fetch_ not on the configuration being unset but on the file::read failing, which seems like mistakenly ignoring an error. Eventually, we want to persist the response from fetch_ so that fetch_ isn't called every time the program is run. If that were the behavior, then it would make sense to have a default filepath that fetches if it doesn't exist and fails if overridden but unable to read. I recognize this peels back way more layers than the original issue but I hope this explains the desired behavior sufficiently to inform you on the next steps you feel like you can take.

Since the overarching theme is to pay back tech debt, it might make more sense to start with the ConfigBuilder pattern instead of set, and then coming back to fixing this error propagation. On the other hand if it's easier to get this error working short term that lets us close this PR and keep the PR queue flowing [Little's law]. What do you think?

[shinghim]

It's late here so I'm reading this while i'm kinda sleepy but that makes sense for the most part. I just ran this command against the changes in this branch and what's currently in master:

cargo run --features v2 -- receive 66666 --ohttp-keys /path

The ohttp-keys argument is a path that didn't exist, and when running on master i get:

[2024-12-13T04:26:09Z ERROR payjoin_cli::app::config] Failed to read ohttp_keys file: No such file or directory (os error 2)
Error: Failed to parse config

Caused by:
    invalid type: string "./path/to/ohttp_keys", expected a sequence

The error wasn't propagated, and it continues to run until it tries building the Config from config.toml, where ohttp_keys was set to ./path/to/ohttp_keys

In the run with these PR changes, the output is now:

[2024-12-13T04:26:02Z ERROR payjoin_cli::app::config] Failed to read ohttp_keys file: No such file or directory (os error 2)
Error: Failed to parse config

Caused by:
    Failed to read ohttp_keys file: No such file or directory (os error 2)

so the error is propagated and the user will now know that the read failed instead of getting a somewhat unrelated error of a different path not being a sequence

[shinghim]

Sorry for the long discussion for this task. For some reason I couldn't initially figure out the ArgMatches stuff and i couldn't let it go so i focused on that rather than the actual goal of the PR 😅

[DanGould]

I see and was able to test as you have to success when using the cli argument, however, I get the "expected a sequence" error when setting ohttp_keys ="path" in config.toml. I'm going to propose change addresses the deserialization that fixes both. Without this custom deserializer, the config expects a "sequence" (bytes for a type). I think this makes it work for both. Let me know if it works for you.

#[serde(deserialize_with = "deserialize_ohttp_keys_from_path")]
pub ohttp_keys: Option<payjoin::OhttpKeys>,

#[cfg(feature = "v2")]
let ohttp_keys = matches.get_one::<String>("ohttp_keys").map(|s| s.as_str());

(it might make sense to combine the above and below lines as is done in other parts of the config)

.set_override_option("ohttp_keys", ohttp_keys)?

#[cfg(feature = "v2")]
fn deserialize_ohttp_keys_from_path<'de, D>(
    deserializer: D,
) -> Result<Option<payjoin::OhttpKeys>, D::Error>
where
    D: serde::Deserializer<'de>,
{
    let path_str: Option<String> = Option::deserialize(deserializer)?;

    match path_str {
        None => Ok(None),
        Some(path) => std::fs::read(path)
            .map_err(|e| serde::de::Error::custom(format!("Failed to read ohttp_keys file: {}", e)))
            .and_then(|bytes| {
                payjoin::OhttpKeys::decode(&bytes).map_err(|e| {
                    serde::de::Error::custom(format!("Failed to decode ohttp keys: {}", e))
                })
            })
            .map(Some),
    }
}

[DanGould]

Sometimes it just takes a discussion to hash things out. Best to put the conclusion of this discussion in a commit message so that the next person who comes to find it understands what the heck we were talking about 😄

[shinghim]

Nice, I like the custom deserializer solution! Two birds with one stone. I'll update here in a bit

[shinghim]

Having some trouble with the test in payjoin-cli/tests/e2e.rs after adding the deserializer:

Error: Failed to parse config

Caused by:
    missing field `ohttp_keys`

Since ohttp_keys is an Option<>, it shouldn't throw this error, right?

[DanGould]

If you can push the version you think is broken I can check it within 12 hours

[shinghim]

Pushed - the changes are what you've suggested

[DanGould]

I think there was default None wasn't explicitly set, so when there was nothing to deserialize there was a problem. This is the fix. Please fix it up into your commit so we have a history that builds properly and passes tests on each commit

Not quite 12 hours, but 24! 😅

[shinghim]

Ahh i misunderstood the error. I didn't realize ohttp_keys wasn't in there at all instead of it just being None, but makes sense! Tests are passing now