Consider generalizing `Uri` deserialization

[shinghim]

fwiw the ser and de modules are pub mod, so they are part of the public API.

if the deserialization impl was generalized from Uri to a pub trait that Uri impls, that consumes a stream of Param values, then the low level API would already be supported through this hypothetical trait without completely redoing the high level API

having that supported would further justify high level ergonomic features like treating req- as a modifier, not part of the parameter name.

should we move this discussion to a new issue in the bitcoin_uri repo?

Originally posted by @nothingmuch in #433

[shinghim]

@nothingmuch

Trying to see if I understand what you mean: is the idea to generalize the deserialization of all params, including amount, label, and message along with the extras, and handling them all the same i.e. checking for duplicates, checking for req- prefixes, etc?

[nothingmuch]

taking just deserialization as an example, de.rs defines an impl block for the type Uri<'a, NetworkUnchecked, T> where T is the extras.

https://github.com/payjoin/bitcoin_uri/blob/master/src/de.rs#L20

Uri contains 4 concrete fields and a generic extras field:

bitcoin_uri/src/lib.rs

Lines 83 to 103 in 9362fab

	pub struct Uri<'a, NetVal = bitcoin::address::NetworkChecked, Extras = NoExtras>
	where
	NetVal: NetworkValidation,
	{
	/// The address provided in the URI.
	///
	/// This field is mandatory because the address is mandatory in BIP21.
	pub address: bitcoin::Address<NetVal>,
	/// Number of satoshis requested as payment.
	pub amount: Option<bitcoin::Amount>,
	/// The label of the address - e.g. name of the receiver.
	pub label: Option<Param<'a>>,
	/// Message that describes the transaction to the user.
	pub message: Option<Param<'a>>,
	/// Extra fields that can occur in a BIP21 URI.
	pub extras: Extras,
	}

the Uri struct is an accumulator or values extracted from a stream of tokens in a string, as parsed here:

https://github.com/payjoin/bitcoin_uri/blob/master/src/de.rs#L23-L37

and here:

bitcoin_uri/src/de.rs

Lines 55 to 82 in 9362fab

	for param in params.split('&') {
	let pos = param
	.find('=')
	.ok_or_else(|| Error::Uri(UriError(UriErrorInner::MissingEquals(param.to_owned()))))?;
	let key = &param[..pos];
	let value = &param[(pos + 1)..];
	match key {
	"amount" => {
	let parsed_amount = bitcoin::Amount::from_str_in(value, Denomination::Bitcoin).map_err(Error::uri)?;
	amount = Some(parsed_amount);
	},
	"label" => {
	let label_decoder = Param::decode(value).map_err(Error::percent_decode_static("label"))?;
	label = Some(label_decoder);
	},
	"message" => {
	let message_decoder = Param::decode(value).map_err(Error::percent_decode_static("message"))?;
	message = Some(message_decoder);
	},
	extra_key => {
	let decoder = Param::decode(value).map_err(Error::percent_decode(key))?;
	let is_known = deserializer.deserialize_borrowed(extra_key, decoder).map_err(Error::Extras)?;
	if is_known == ParamKind::Unknown && extra_key.starts_with("req-") {
	return Err(Error::Uri(UriError(UriErrorInner::UnknownRequiredParameter(extra_key.to_owned()))));
	}
	},
	}
	}

each of the 4 fields starts at None, and is set to Some(v) when a param with the matching key is found.

unrecognized fields are handled by the deserializer, which comes from an associated type of T, which allows accumulation of those values.

bitcoin_uri/src/de.rs

Lines 75 to 79 in 9362fab

	let decoder = Param::decode(value).map_err(Error::percent_decode(key))?;
	let is_known = deserializer.deserialize_borrowed(extra_key, decoder).map_err(Error::Extras)?;
	if is_known == ParamKind::Unknown && extra_key.starts_with("req-") {
	return Err(Error::Uri(UriError(UriErrorInner::UnknownRequiredParameter(extra_key.to_owned()))));
	}

so I'm proposing this method be split up into two layers of abstraction:

1. a pub fn deserialize_raw (renamed to tokenize_params maybe?) that
   • either operates on a &'a str argument, which produces something like an iterator of Result<Param(&'a str), E> where i guess Err(E) terminates the iteration
   • or as a simpler alternative accepts an additional trait ParamHandler { parse_param(Param) } argument
     • keeping the eager behavior would simplify error handling, it can just return Result<ParamHandler::SomeAssociatedReturnValueType, E>
     • this makes sense since URIs are size constrained so streaming reader is needlessly complicated
     • and the type already assumes a &str input, and this can preserve the heap allocation avoidance that's already in the current behavior
2. impl ParamHandler<'a> for Uri<'a, NetworkChecked, T>, where parse_param or whatever it's called contains the match block that checks the parameter keys, and applies them to the Uri type and/or the extras field.

this way people can implement their own alternative to Uri that accumulates the values differently, without needing to reimplement the parsing logic of deserialize_raw, which could be made pub. deserialize_raw is just a tokenizer that generates a stream of key value pairs for an arbitrary parser to interpret, but preserving the public API that Uri already has (which calls deserialize_raw via TryFrom<&str>.

[nothingmuch]

I do feel the need to add that I don't see a reason to implement this without a clear use case, since without one it just bloats the public API

[shinghim]

That makes sense - in the case of payjoin, we'd have our own PayjoinUri, but I agree that a clear use case should come before implementing this. Should we keep this open just in case that use case arises since you've already put some thoughts into how it might be done?

[nothingmuch]

That makes sense - in the case of payjoin, we'd have our own PayjoinUri, but I agree that a clear use case should come before implementing this

I think it can probably remain Uri<_, _, PayjoinExtras> or am I missing something?

Should we keep this open just in case that use case arises since you've already put some thoughts into how it might be done?

Yeah, I mean the main point of thinking that through was to have the confidence that it's straightforward, so that we can double down on more ergonomic Uri type that rejects duplicate parameters or other edge cases like duplicate keys that are not strictly prohibited by the spec, or treating the parameter set as a map instead of as an unordered list of pairs

[shinghim]

I think it can probably remain Uri<_, _, PayjoinExtras> or am I missing something?

no you're right, im not sure what i was thinking there when i wrote that.

For the issue from payjoin/rust-payjoin#433 though, should we just keep handling the duplicates within rust-payjoin for now?

[nothingmuch]

For the issue from payjoin/rust-payjoin#433 though, should we just keep handling the duplicates within rust-payjoin for now?

I don't think we can reliably without exposing this low level API since the params that map to the built in fields of Uri have no such handling, so I would prefer if we modify bitcoin_uri, exactly as you have in #9