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.

Sign up for GitHub

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

Jump to bottom

Integrate sdk-common #308

Merged
merged 30 commits into from
Jun 20, 2024
Merged

Integrate sdk-common #308

merged 30 commits into from
Jun 20, 2024

Conversation

ok300
Copy link
Contributor

@ok300 ok300 commented Jun 12, 2024 •
edited
Loading

  • parse_invoice
  • parse
  • LNURL-pay
    • Store success action in separate table, linked to SendSwap by payment_hash
    • Update min/max range for LnUrlPayRequestData to also reflect swap min/max
  • LNURL-withdraw
  • LNURL-auth
    • TBD: Do we directly want to access the seed? Or better export this whole method upstream to lwk?

Closes #275, #276

Depends on breez/breez-sdk-greenlight#1012

@ok300 ok300 force-pushed the ok300-integrate-sdk-common branch from abb09c9 to 82fb793 Compare June 12, 2024 13:26
@ok300 ok300 force-pushed the ok300-integrate-sdk-common branch from 9cf9e6f to 3f42fab Compare June 13, 2024 08:54
@ok300 ok300 marked this pull request as ready for review June 18, 2024 10:31
roeierez
roeierez requested changes Jun 18, 2024
View reviewed changes
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. Dropped some comments.

lib/core/src/lib.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
@roeierez
Copy link
Member

@ok300 for the open points:

  1. Success action stored in a separate table seems correct to me
  2. min/max range should IMO reflect only the LNURL, otherwise the user won't know the service original result
  3. for deriving I think indeed better not to expose the seed directly but can be done in a separate PR.

dangeross
dangeross reviewed Jun 19, 2024
View reviewed changes
lib/core/Cargo.toml Outdated Show resolved Hide resolved
@ok300 ok300 self-assigned this Jun 19, 2024
ok300 added 3 commits June 19, 2024 19:34
@ok300
Merge branch 'main' into ok300-integrate-sdk-common
48fd14b 
# Conflicts:
#	lib/bindings/langs/flutter/breez_liquid_sdk/include/breez_liquid_sdk.h
#	lib/core/src/frb_generated.io.rs
#	lib/core/src/frb_generated.rs
#	lib/core/src/model.rs
#	lib/core/src/sdk.rs
#	lib/core/src/swapper/mod.rs
#	lib/core/src/test_utils.rs
#	packages/dart/lib/src/bindings.dart
#	packages/dart/lib/src/frb_generated.dart
#	packages/dart/lib/src/frb_generated.io.dart
#	packages/dart/lib/src/model.dart
#	packages/flutter/lib/flutter_breez_liquid_bindings_generated.dart
#	packages/react-native/android/src/main/java/com/breezliquidsdk/BreezLiquidSDKMapper.kt
#	packages/react-native/ios/BreezLiquidSDKMapper.swift
#	packages/react-native/src/index.ts
@ok300
Move FRB mirroring structs to bindings.rs
db821d9
@ok300
Remove TODO
d237d23
@ok300
Copy link
Contributor Author

ok300 commented Jun 19, 2024

@roeierez to your points:

  1. Success action stored in a separate table seems correct to me

Opened follow-up #318

  1. min/max range should IMO reflect only the LNURL, otherwise the user won't know the service original result

Agree, will then leave it as it is.

  1. for deriving I think indeed better not to expose the seed directly but can be done in a separate PR.

Opened follow-up #317

I opened those as follow-ups so we can get this merged, as it's the basis for other changes (for example the proxy swapper).

@ok300 ok300 requested review from roeierez and dangeross June 19, 2024 19:01
roeierez
roeierez requested changes Jun 19, 2024
View reviewed changes
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.

Other than one sugestion for getting rid of rusqlite, LGTM.

lib/core/Cargo.toml Outdated Show resolved Hide resolved
dangeross
dangeross reviewed Jun 20, 2024
View reviewed changes
Copy link
Collaborator

@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.

Looks good. I think the example Flutter and RN apps need updating with the Network rename

@ok300 ok300 linked an issue Jun 20, 2024 that may be closed by this pull request
Implement LNURL-Withdraw #276
Closed
@ok300 ok300 mentioned this pull request Jun 20, 2024
Merged
roeierez
roeierez approved these changes Jun 20, 2024
View reviewed changes
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

@ok300 ok300 merged commit bcb4743 into main Jun 20, 2024
6 checks passed
@ok300 ok300 deleted the ok300-integrate-sdk-common branch June 20, 2024 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dangeross dangeross dangeross left review comments

@roeierez roeierez roeierez approved these changes

@hydra-yse hydra-yse Awaiting requested review from hydra-yse

Assignees

@ok300 ok300

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement LNURL-Withdraw Implement Lnurl pay
3 participants
@ok300 @roeierez @dangeross