Implement returning payment hash (#1159) #1166
Conversation
…h return and updates related dependencies/files.
There was a problem hiding this comment.
Thanks for the PR!
There's quite a few changes unrelated to the PR. Some I've listed in comments. The following files shouldn't need any changes, can you remove those changes please?
Please remove the changes to
- .gitignore
- libs/Cargo.lock
- libs/sdk-bindings/bindings-android/buildForJitpack.sh
- libs/sdk-bindings/bindings-android/gradle/wrapper/gradle-wrapper.jar
- libs/sdk-bindings/bindings-android/gradlew
- libs/sdk-bindings/bindings-android/gradlew.bat
- libs/sdk-bindings/bindings-csharp/src/.gitignore
- libs/sdk-bindings/bindings-csharp/src/Breez.Sdk.csproj
- libs/sdk-bindings/bindings-csharp/test/.gitignore
- libs/sdk-bindings/bindings-csharp/test/Program.cs
- libs/sdk-bindings/bindings-csharp/test/sdk-cs-demo.csproj
- libs/sdk-bindings/bindings-kotlin-multiplatform/gradle/wrapper/gradle-wrapper.jar
- libs/sdk-bindings/bindings-kotlin-multiplatform/gradlew
- libs/sdk-bindings/bindings-kotlin-multiplatform/gradlew.bat
- libs/sdk-bindings/tests/bindings/csharp/.gitignore
- libs/sdk-bindings/tests/bindings/csharp/Program.cs
- libs/sdk-bindings/tests/bindings/csharp/sdk-cs-demo.csproj
- libs/sdk-common/Cargo.toml
- libs/sdk-common/build.rs
- libs/sdk-common/src/breez_server.rs
- libs/sdk-common/src/buy/moonpay.rs
- libs/sdk-common/src/fiat.rs
- libs/sdk-common/src/grpc/breez.rs
- libs/sdk-common/src/grpc/mod.rs
- libs/sdk-common/src/grpc/proto/breez.proto
- libs/sdk-common/src/model.rs
- libs/sdk-core/src/breez_services.rs
- libs/sdk-core/src/persist/send_pays.rs
- libs/sdk-core/src/swap_out/reverseswap.rs
- libs/sdk-react-native/android/gradle/wrapper/gradle-wrapper.jar
- libs/sdk-react-native/android/gradlew
- libs/sdk-react-native/android/gradlew.bat
- libs/sdk-react-native/example/android/gradle/wrapper/gradle-wrapper.jar
- libs/sdk-react-native/example/android/gradlew
- libs/sdk-react-native/example/android/gradlew.bat
Perhaps you were targeting the wrong branch with your PR?
| string src_node_id; | ||
| string short_channel_id; | ||
| u32 fees_base_msat; | ||
| u32 fees_proportional_millionths; | ||
| u64 cltv_expiry_delta; | ||
| u64? htlc_minimum_msat; | ||
| u64? htlc_maximum_msat; | ||
| }; |
There was a problem hiding this comment.
This whitespace change is not needed
libs/sdk-bindings/src/breez_sdk.udl
Outdated
| Payment payment; | ||
| Optional<SuccessActionProcessed> success_action; |
There was a problem hiding this comment.
The success_action field doesn't need a change here? Does the order matter?
At least it should be SuccessActionProcessed? success_action;
There was a problem hiding this comment.
Thanks for catching that! You're right, the correct syntax should be SuccessActionProcessed? success_action; to use UDL's optional syntax.
| @@ -918,7 +918,7 @@ interface BlockingBreezServices { | |||
|
|
|||
| [Throws=SdkError] | |||
| SwapInfo? in_progress_swap(); | |||
|
|
|||
There was a problem hiding this comment.
Whitespace change not needed
| @@ -974,7 +974,7 @@ interface BlockingBreezServices { | |||
| PrepareRedeemOnchainFundsResponse prepare_redeem_onchain_funds(PrepareRedeemOnchainFundsRequest req); | |||
| }; | |||
|
|
|||
| namespace breez_sdk { | |||
| namespace breez_sdk { | |||
There was a problem hiding this comment.
Whitespace change not needed
| }, | ||
| LnUrlPayResult::EndpointSuccess { data } => { | ||
| let expected_hash = inv.payment_hash().to_hex(); | ||
| assert_eq!(data.payment_hash, expected_hash); |
| @@ -26,6 +26,7 @@ pub enum LnUrlPayResult { | |||
| pub struct LnUrlPaySuccessData { | |||
| pub payment: Payment, | |||
| pub success_action: Option<SuccessActionProcessed>, | |||
| pub payment_hash: String, // Add payment_hash field | |||
There was a problem hiding this comment.
| pub payment_hash: String, // Add payment_hash field | |
| pub payment_hash: String, |
| @@ -659,6 +659,7 @@ pub enum PaymentStatus { | |||
| #[derive(PartialEq, Eq, Debug, Clone, Serialize, Deserialize)] | |||
| pub struct Payment { | |||
| pub id: String, | |||
| pub payment_hash: String, // Explicitly adding the payment_hash field | |||
There was a problem hiding this comment.
| pub payment_hash: String, // Explicitly adding the payment_hash field | |
| pub payment_hash: String, |
There was a problem hiding this comment.
Thanks for your feedback! I see your point about the payment_hash, but it's actually supposed to be there. It's how we keep track of each payment.
|
Hi @JssDWt, thank you for the review, I have fixed the success_action type, removed the extra payment_hash field in lnurl/pay.rs, and confirmed that the payment_hash in models.rs is correct. I've also reverted the changes to the unrelated files you pointed out. |
|
@Patti88 If you look at the 'files changed' tab, there's still 38 changed files. Can you bring that down to only the relevant files? When that's done please squash the commits in a single commit. You could use Github Desktop for example, it has a nice diff viewer and allows you to squash commits by selecting them and right-clicking. |
|
The diff is still very large and not relevant. |
|
I see the diff has been much improved. Now I can see the trees in the forest. Let me review the code. You're creating a fix for this issue. That function is here in the udl file. It should return a response object, rather than The corresponding function implementation is here. That should return that new object. You can retrieve the payment hash by hashing the preimage, or decoding the invoice. Decoding the invoice is probably the preferred way. Currently this PR changes the response of the I would suggest to start fresh with this PR, implementing the required changes for the mentioned issue. |
No description provided.