-
Notifications
You must be signed in to change notification settings - Fork 72
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
#336 followups #351
#336 followups #351
Conversation
4419f46
to
85a1d01
Compare
Hmm, the flaky python test seems unrelated, at least seems to work fine locally. |
85a1d01
to
2dabdc3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the help! LGTM
2dabdc3
to
775f8b9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Please squash.
Previously, the `SendingParamters` field was simply an `Option<u64>`, which however means we could just override it to be `Some`. Here, we have it be `Option<Option<u64>>` which allows the `None` override. As UniFFI doesn't support `Option<Option<..>>`, we work around this via a dedicated `enum` that is only exposed under the `uniffi` feature.
.. as it was used for spontaneous payments only and hence a bit misleading. We drop it for now and see if any users would complain. If so, it would probably be sufficient for it to be an optional parameter on the spontaneous payments methods.
775f8b9
to
b282d42
Compare
Squashed without further changes. |
.. a few minor follow-ups to #336.
(cc @slanesuke)