Return error for duplicate concrete params

[shinghim]

No description provided.

[nothingmuch]

utACK

i think it makes sense to also match either amount or req-amount, and same for label and message, arguably as a separate PR, it's weird to me that something like req-amount would be delegated to the Extras type and it makes sense to me that req-amount and amount both present in the same URI would be considered a DuplicateParameter("amount").

[nothingmuch]

i think this could safely be &static str because the expected parameter keys are hard coded, undecided if I prefer this for simplicity & consistency with UnknownRequiredParameter which I think kinda needs to have a String since it makes sense for the error to outlive the erroneous input

[shinghim]

I had this exact thought and went with String for consistency with UnknownRequiredParameter

[shinghim]

@nothingmuch agreed, though when we start handling required params, we'll need some way of conveying if it's required or optional

[spacebear21]

utACK 22d109f.

Is there a rationale for not checking duplicates on any extra parameter?

[shinghim]

Is there a rationale for not checking duplicates on any extra parameter?

the only reason is that it's being done already by the implementors of the Extra, at least for Payjoin's case:

https://github.com/payjoin/rust-payjoin/blob/affd9d93df770e5865a1d6771f92d2aa87bd88cf/payjoin/src/uri/mod.rs#L168

If we eventually handle it here, we can remove the duplicate handling there

[spacebear21]

BIP21 doesn't specify behavior for duplicate parameters... However, BIP321 says:

Multiple query parameters with the same key MAY be included for query parameters representing payment instructions. Multiple query parameters with the same key MUST NOT be included for keys "label", "message", or "pop". Multiple query parameters with the same key for other keys MUST be allowed for unknown query parameters. Future query parameter keys may or may not allow for duplicate parameters.

So unknown keys should be left to be handled by implementations. Not sure why "amount` is not included in that paragraph, but the invalid URLs example section states that "Amounts must not appear twice".

[spacebear21]

@shinghim It failed some lint checks that I had to run manually, can you fix those please?

[shinghim]

Done. They weren't related to the duplicate checks so i put it in a new commit

[spacebear21]

There is also a failing test but it appears unrelated as well so I'll merge this as is.