Skip to content

Conversation

@tnull
Copy link
Collaborator

@tnull tnull commented Aug 27, 2024

.. a few minor follow-ups to #336.

(cc @slanesuke)

@tnull tnull force-pushed the 2024-08-336-followup branch from 4419f46 to 85a1d01 Compare August 27, 2024 10:12
@tnull tnull requested a review from jkczyz August 27, 2024 10:15
@tnull
Copy link
Collaborator Author

tnull commented Aug 27, 2024

Hmm, the flaky python test seems unrelated, at least seems to work fine locally.

@tnull tnull force-pushed the 2024-08-336-followup branch from 85a1d01 to 2dabdc3 Compare August 27, 2024 12:45
Copy link
Contributor

@slanesuke slanesuke left a 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

@tnull tnull force-pushed the 2024-08-336-followup branch from 2dabdc3 to 775f8b9 Compare August 28, 2024 06:03
Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Please squash.

tnull added 2 commits August 28, 2024 16:10
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.
@tnull tnull force-pushed the 2024-08-336-followup branch from 775f8b9 to b282d42 Compare August 28, 2024 14:10
@tnull
Copy link
Collaborator Author

tnull commented Aug 28, 2024

Squashed without further changes.

@tnull tnull merged commit ab7cc70 into lightningdevkit:main Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants