allow param deletion via Option<&str> in set_param in uri/v2

[AdamuAbba]

Which crate is this feature request for?

payjoin

Please describe the feature you'd like to see added.

There's a TODO in set_param asking whether to allow deleting a URL fragment parameter by changing param to Option<&str>. i'f i understand this correctly, param here means new_param

rust-payjoin/payjoin/src/core/uri/v2.rs

Lines 228 to 255 in a250593

	fn set_param(url: &mut Url, new_param: &str) {
	let fragment = url.fragment().unwrap_or("");
	let delim = check_fragment_delimiter(fragment)
	.expect("set_param must be called on a URL with a valid fragment");
	// In case of an invalid fragment parameter the following will still attempt
	// to retain the existing data
	let mut params = fragment
	.split(delim)
	.filter(|param| !param.is_empty())
	.map(|param| {
	let key = param.split('1').next().unwrap_or(param);
	(key, param)
	})
	.collect::<BTreeMap<&str, &str>>();
	// TODO: change param to Option(&str) to allow deletion?
	let key = new_param.split('1').next().unwrap_or(new_param);
	params.insert(key, new_param);
	if params.is_empty() {
	url.set_fragment(None)
	} else {
	// Can we avoid intermediate allocation of Vec, intersperse() exists but not in MSRV
	let fragment = params.values().copied().collect::<Vec<_>>().join("-");
	url.set_fragment(Some(&fragment));
	}
	}

There are three afected call sites if the function where to be modified

1. rust-payjoin/payjoin/src/core/uri/v2.rs

   Lines 33 to 40 in a250593

   	fn set_receiver_pubkey(url: &mut Url, pubkey: &HpkePublicKey) {
   	let rk_hrp: Hrp = Hrp::parse("RK").expect("parsing a valid HRP constant should never fail");
   	set_param(
   	url,
   	&crate::bech32::nochecksum::encode(rk_hrp, &pubkey.to_compressed_bytes())
   	.expect("encoding compressed pubkey bytes should never fail"),
   	)
   	}

2. rust-payjoin/payjoin/src/core/uri/v2.rs

   Line 60 in a250593

   fn set_ohttp(url: &mut Url, ohttp: &OhttpKeys) { set_param(url, &ohttp.to_string()) }

3. rust-payjoin/payjoin/src/core/uri/v2.rs

   Lines 80 to 87 in a250593

   	fn set_expiration(url: &mut Url, exp: &Time) {
   	let ex_hrp: Hrp = Hrp::parse("EX").expect("parsing a valid HRP constant should never fail");
   	let exp_str = crate::bech32::nochecksum::encode(ex_hrp, &exp.to_bytes())
   	.expect("encoding u32 timestamp should never fail");
   	set_param(url, &exp_str)
   	}

Is your feature related to a problem, if so please describe it.

No response

Describe the solution you'd like

So If this is worth considering now just changing new_param to Option<&str> on its own isn't enough since The key is currently derived from the value itself:

/// here
let key = new_param.split('1').next().unwrap_or(new_param);

and with None there is no value to split, so there is no key to remove. The caller has to supply it. We are gonna need to add a key parameter.

   /// Set a URL fragment parameter, inserting it or replacing it depending on
   /// whether a parameter with the same bech32 HRP is already present.
+  /// Passing `None` removes the parameter with that HRP, if present.
   ///
   /// Parameters are sorted lexicographically by prefix.
-  fn set_param(url: &mut Url, new_param: &str) {
+  fn set_param(url: &mut Url, key: &str, new_param: Option<&str>) {                                                                      󰊓
       let fragment = url.fragment().unwrap_or("");
       let delim = check_fragment_delimiter(fragment)
           .expect("set_param must be called on a URL with a valid fragment");

       // In case of an invalid fragment parameter the following will still attempt
       // to retain the existing data
       let mut params = fragment
           .split(delim)
           .filter(|param| !param.is_empty())
           .map(|param| {
               let key = param.split('1').next().unwrap_or(param);
               (key, param)
           })
           .collect::<BTreeMap<&str, &str>>();

-      // TODO: change param to Option(&str) to allow deletion?
-      let key = new_param.split('1').next().unwrap_or(new_param);
-      params.insert(key, new_param);
+      match new_param {
+          Some(new_param) => params.insert(key, new_param),
+          None => params.remove(key),
+      };

       if params.is_empty() {
           url.set_fragment(None)
       } else {
           // Can we avoid intermediate allocation of Vec, intersperse() exists but not in MSRV
           let fragment = params.values().copied().collect::<Vec<_>>().join("-");
           url.set_fragment(Some(&fragment));
       }
   }

The cost of this change tho is that all three call sites i mentioned earlier set_receiver_pubkey,set_ohttp,set_expiration grow a Some(..) wrapper plus an "RK" / "OH" / "EX" literal that duplicates the Hrp constant parsed a line or two above. personally i feel like it's a lot of changes for one functionality but yeah let me know what you guys think, point me in the right direction or how you would advise that i approach this but if this isn't useful at the moment we can close this issue for now. thank you.

A.I disclaimer: I used Claude to understand the code context and generate the diffs.

Describe any alternatives you've considered

No response

Please leave any additional context

No response

[benalleng]

I still think there needs to be a "WHY" articulated before we move forward on a PR for this. I think part of the reason why this was left as a TODO is that its not clear what param deletion would really be useful for. Happy to be corrected but this is my take if anyone else can chime in on a reason for adding the ability to delete individual params