Skip to content
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

Add payment expiry #622

Merged
merged 1 commit into from
Jan 8, 2025
Merged

Add payment expiry #622

merged 1 commit into from
Jan 8, 2025

Conversation

dangeross
Copy link
Collaborator

@dangeross dangeross commented Dec 20, 2024

This PR:

  • exposes the liquid/bitcoin_expiration_blockheight field in the Lightning/Bitcoin type payment details, based on the swap's timeout_block_height field
  • changes the GetInfoResponse structure to include two fields only, the wallet information (pubkey, fingerprint etc.) and the blockchain information (tips), so that clients may compare the current tip to the payment's expiration block height

Fixes #415

@dangeross dangeross force-pushed the savage-payment-expiry branch from 75c048f to 4dbfe3b Compare December 20, 2024 18:08
lib/core/src/persist/migrations.rs Outdated Show resolved Hide resolved
Copy link
Member

@roeierez roeierez left a comment

Choose a reason for hiding this comment

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

Looks good. Some small comments.

lib/core/src/sync/model/data.rs Outdated Show resolved Hide resolved
lib/core/src/recover/recoverer.rs Outdated Show resolved Hide resolved
@roeierez roeierez added this to the v0.6..0 milestone Dec 23, 2024
@dangeross dangeross changed the base branch from savage-lnurl-payment-details to rt-sync December 23, 2024 11:57
@hydra-yse hydra-yse force-pushed the savage-payment-expiry branch from 4dbfe3b to 944211e Compare December 24, 2024 14:53
@hydra-yse hydra-yse changed the base branch from rt-sync to main December 24, 2024 14:53
@hydra-yse hydra-yse requested a review from roeierez December 25, 2024 19:28
lib/core/src/model.rs Outdated Show resolved Hide resolved
lib/core/src/recover/recoverer.rs Outdated Show resolved Hide resolved
@hydra-yse hydra-yse force-pushed the savage-payment-expiry branch 2 times, most recently from 13a4b72 to 812b31f Compare December 27, 2024 11:09
@hydra-yse hydra-yse requested a review from roeierez December 27, 2024 11:13
@hydra-yse hydra-yse force-pushed the savage-payment-expiry branch from 812b31f to 35f41bb Compare December 27, 2024 11:15
@hydra-yse hydra-yse marked this pull request as ready for review December 27, 2024 11:16
lib/core/src/sdk.rs Outdated Show resolved Hide resolved
lib/core/src/model.rs Show resolved Hide resolved
lib/core/src/model.rs Outdated Show resolved Hide resolved
lib/core/src/model.rs Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@dangeross dangeross left a comment

Choose a reason for hiding this comment

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

The tests and Notfication Plugin also need updating to accommodate the get_info() response changes

lib/core/src/model.rs Outdated Show resolved Hide resolved
lib/core/src/model.rs Outdated Show resolved Hide resolved
lib/core/src/recover/recoverer.rs Outdated Show resolved Hide resolved
lib/core/src/recover/recoverer.rs Outdated Show resolved Hide resolved
lib/core/src/sync/model/data.rs Show resolved Hide resolved
lib/core/src/sync/model/data.rs Show resolved Hide resolved
lib/core/src/chain/liquid.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@danielgranhao danielgranhao left a comment

Choose a reason for hiding this comment

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

Other than @dangeross's comments, utACK

lib/core/src/persist/cache.rs Outdated Show resolved Hide resolved
hydra-yse added a commit that referenced this pull request Dec 31, 2024
@hydra-yse hydra-yse force-pushed the savage-payment-expiry branch from a081d15 to 1db411d Compare December 31, 2024 12:09
hydra-yse added a commit that referenced this pull request Jan 2, 2025
@hydra-yse hydra-yse force-pushed the savage-payment-expiry branch from 1db411d to 617a070 Compare January 2, 2025 08:19
@hydra-yse hydra-yse requested a review from roeierez January 2, 2025 08:44
@hydra-yse hydra-yse force-pushed the savage-payment-expiry branch 2 times, most recently from a609502 to e9e9c6b Compare January 2, 2025 09:28
Copy link
Member

@roeierez roeierez left a comment

Choose a reason for hiding this comment

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

LGTM

lib/core/src/model.rs Outdated Show resolved Hide resolved
lib/core/src/model.rs Outdated Show resolved Hide resolved
lib/core/src/model.rs Outdated Show resolved Hide resolved
lib/core/src/model.rs Outdated Show resolved Hide resolved
hydra-yse added a commit that referenced this pull request Jan 4, 2025
@hydra-yse hydra-yse force-pushed the savage-payment-expiry branch from e9e9c6b to 6ddddb1 Compare January 4, 2025 00:47
@dangeross
Copy link
Collaborator Author

dangeross commented Jan 4, 2025

Seems like the Info not found error is causing the bindings tests to fail. I would check that get_info() correctly correctly initialises the wallet info cache if there is no cache set when the fn is called

Copy link
Collaborator Author

@dangeross dangeross left a comment

Choose a reason for hiding this comment

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

LGTM. Just needs a rebase

We should note its a breaking change in the release @roeierez

hydra-yse pushed a commit that referenced this pull request Jan 6, 2025
fix(clippy): use `clone_if_set` for sync destination

fix: revert rebase changes

feat: add `expiration_block` to payment details

feat: add `blockchain_details` to `GetInfoResponse`

fix: rename fields to `<chain>_expiry_blockheight`

fix: update `GetInfoResponse` structure

fix: address review comments

taken from
#622 (review)

fix: use better error handling for get_info

fix: bindings tests

fix: ensure blockchain_info is optional in query
@hydra-yse hydra-yse force-pushed the savage-payment-expiry branch from e3c89d5 to c74d8f0 Compare January 6, 2025 10:32
@roeierez
Copy link
Member

roeierez commented Jan 6, 2025

@hydra-yse we need a following PR in misty breez and change the docs accordingly. Let's hold the merge at least until we have the misty breez change which should be straight forward otherwise the misty build will be broken.

hydra-yse added a commit to breez/misty-breez that referenced this pull request Jan 7, 2025
hydra-yse added a commit to breez/misty-breez that referenced this pull request Jan 7, 2025
@hydra-yse hydra-yse mentioned this pull request Jan 7, 2025
3 tasks
fix(clippy): use `clone_if_set` for sync destination

fix: revert rebase changes

feat: add `expiration_block` to payment details

feat: add `blockchain_details` to `GetInfoResponse`

fix: rename fields to `<chain>_expiry_blockheight`

fix: update `GetInfoResponse` structure

fix: address review comments

taken from
#622 (review)

fix: use better error handling for get_info

fix: bindings tests

fix: ensure blockchain_info is optional in query
@hydra-yse hydra-yse force-pushed the savage-payment-expiry branch from c74d8f0 to 287746d Compare January 7, 2025 23:42
@hydra-yse hydra-yse merged commit 66810ec into main Jan 8, 2025
9 checks passed
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.

Add expiration time to Payment for pending payments
4 participants