Encode fields in opening_fee_params promise

[JssDWt]

This PR makes the opening_fee_params objects extensible for future backward compatible upgrades. Fixes #94

The problem

If this spec ever decides to add an additional field in the opening_fee_params object, this upgrade path is impossible to roll out in the current spec, without placing too much logic on the client side.

LSP is upgraded, client is not

• The LSP constructs a new opening_fee_params object, containing the new field. Commits to the object in the promise.
• The client deserializes the object, not knowing about the new field
• The client serializes the object again and sends it back to the LSP without the new field
• The LSP is now unable to verify the promise

Note that it would be possible for the client to send back the opening_fee_params object using exactly the bytes it received from the LSP. But this is a 'hard' constraint, because this would force the client to pass to the UI layer the deserialized object plus an additional field containing the raw opening_fee_params bytes.

The solution

Encode all fields into the promise itself. This way, the promise alone is enough for the LSP to retrieve what it committed to. And the client really only needs to send back the promise to the LSP.

LSP is upgraded, client is not

• The LSP constructs a new opening_fee_params object, containing the new field. Commits to the object in the promise.
• The client deserializes the object, not knowing about the new field.
• The client sends promise to the LSP, this promise contains the new field encoded.
• The LSP is now able to verify the promise.

Notes

• I removed the constraint that the LSP is not allowed to add any fields to the opening_fee_params object, because the LSP could commit to any data unknown to the client, even outside of these communications. SO I don't think it adds much value to have this constraint in the spec.
• I modified the name of the invalid_opening_fee_params error to invalid_promise, keeping the same error number
• This is a breaking change, we discussed during the in-person meeting in Viareggio, this would be the last breaking change to LSPS2. Thankfully this change may help avoid breaking changes in the future when LSPS2 may already be used by multiple implementations.

[tnull]

I'm still not entirely convinced this is entirely necessary and solves the fundamental issue, as the client would still run into issues if it doesn't know a field that influences the opening fee calculation, which is exactly what opening_fee_params are (granted, mostly by now) for. So, in my understanding, anything we would potentially add to opening_fee_params would result in a breaking change anyways and hence we might not ever want to add any more fields to it after stabilization.

Note that the proposed change makes the actual buy request a human unreadable message and also introduces additional complexity and asymmetry in the sense that the client now wouldn't just mirror back the object it receives.

That said, besides my general concerns regarding the approach, the concrete changes look good, if we really want to go that way.

[JssDWt]

Note that the proposed change makes the actual buy request a human unreadable message and also introduces additional complexity and asymmetry in the sense that the client now wouldn't just mirror back the object it receives.

Could you elaborate what you mean by this? In my view the complexity is in passing back the human-readable/deserializable message back to the LSP.

[tnull]

Could you elaborate what you mean by this? In my view the complexity is in passing back the human-readable/deserializable message back to the LSP.

Well you still need to keep the human-readable message around to calculate the fee, validate the LSP didn't withhold more than we expect it to, and show to the user which of the opening fee params was picked. As the client still won't to know how to deal with any new fields when the LSP upgrades, in my view nothing about the 'problem' is actually resolved, but we now reply with a human-unreadable message that is different, i.e., needs to be implemented as yet another object for serialization, where previously we could just reuse the one from GetInfoResponse (granted, not a lot additional complexity).

[tnull]

In any case, while I'm still a bit dubious if this gains us much, I definitely won't block this PR as it also doesn't have a lot of drawbacks. Happy to ACK if others think this is beneficial.

[SeverinAlexB]

I reviewed the changes and I agree with tnull's comment #111 (review). The changes look good but I am unsure if necessary. Because we're already working on the successor of this spec (LSPS4), I am inclined to error on simplicity and hope that there will be no breaking change in LSPS2. On the other hand, I don't mind acking this if Jesse think this is necessary.

So TLDR: I have no strong opinion either way.

[JssDWt]

It was discussed in the LSP spec group call that

• changing the opening_fee_params fields would have effects on fee calculation for clients, so backward compatible changes are 'hard' anyway.
• if it is decided a field needs to be added to the opening_fee_params object, we can decide to encode the fields into the promise then. This still makes the LSP backwards compatible. So we don't have to break anything now.

So I'm closing this PR.