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 listpaymentsincoinjoin and cancelpaymentincoinjoin RPC calls #11976

Merged
merged 8 commits into from
Dec 14, 2023

Conversation

lontivero
Copy link
Collaborator

@lontivero lontivero commented Nov 21, 2023

$ curl -s \
  --user wasabiuser:wasabiuserpassword \
  --data-binary '{"jsonrpc":"2.0", "id":"curltext", "method":"listpaymentsincoinjoin", "params":[]}' \
  -H -- 'content-type: text/plain;' \
  http://127.0.0.1:37128/small/
{
  "jsonrpc": "2.0",
  "result": [
    {
      "id": "4c30f241-64dd-401f-8f56-e8a8424b6173",
      "amount": 755000,
      "destination": "00141d6612643900d85682110e8afc204a04940a2ad2",
      "state": [
        {
          "status": "Pending"
        }
      ],
      "address": "tb1qpzr52rv98f768el3nfqzyjxsgz2qj2q52kj843"
    },
    {
      "id": "d4741809-c8bf-4c45-a70b-4f80ce0ec611",
      "amount": 100000,
      "destination": "0014112468204a04940a2ad23d62110e8afc66900d85",
      "state": [
        {
          "status": "Finished",
          "txid": "8b77030bda186865bb526224e15c3cb51c6b5b32e7a07523ed3f9311da71c5be"
        },
        {
          "status": "In progress",
          "round": "c960f9a1d0d71af9385c38c26e9865ce6353c306a7b058ce68635e3b6d0c8749"
        },
        {
          "status": "Pending"
        }
      ],
      "address": "tb1qzyjxsgz2qj2q52kj843pzr52l3nfqrv98f768e"
    }
  ],
  "id": "curltext"
}

@lontivero lontivero changed the title Add listpayments RPC call Add listpayments and cancelpayment RPC call Nov 23, 2023
@lontivero lontivero marked this pull request as ready for review November 23, 2023 18:35
Copy link
Collaborator

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

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

tested lgtm

  • payincoinjoin amount unit is in BTC? I thought it was in sats
    good to have it the same unit as listpayments (and all other RPC are also in sats)
    maybe could add the fix here

  • listpendingpayments/listpayincoinjoin instead of listpayments feels more intuitive to me, as listpayments can also mean just the wallet history outgoing tx.

  • Maybe write to the logs when payment is cancelled/removed?
    as we also write to log when payment get "scheduled"

@lontivero
Copy link
Collaborator Author

@MarnixCroes, I renamed the methods:

  • listpayments -> listpaymentsincoinjoin
  • cancelpayment -> cancelpaymentincoinjoin

The logs look as follow now:

2023-11-24 10:45:35.613 [15] INFO       PaymentBatch.AddPayment (36)    Payment d2e3497f-c07e-4d7f-8602-891811ca6720 for 1.00000000 BTC to 0 112468204a04940a2ad23d62110e8afc66900d85.
2023-11-24 10:46:12.477 [12] INFO       PaymentBatch.AbortPayment (59)  Payment d2e3497f-c07e-4d7f-8602-891811ca6721 was not found.
2023-11-24 10:46:18.256 [14] INFO       PaymentBatch.AbortPayment (49)  Payment d2e3497f-c07e-4d7f-8602-891811ca6720 for 1.00000000 BTC to 0 112468204a04940a2ad23d62110e8afc66900d85 was canceled.

I didn't get the issue about amounts? What do you mean with that? Can you give me an example?

@MaxHillebrand
Copy link
Collaborator

MaxHillebrand commented Nov 24, 2023

:s/1.00000000 BTC/100000000 sats

@MarnixCroes
Copy link
Collaborator

MarnixCroes commented Nov 24, 2023

I didn't get the issue about amounts? What do you mean with that? Can you give me an example?

ok, it's unrelated to this PR: for payincoinjoin if I enter amount 1, it registers 1 BTC, instead of 1 sat.
arguably it should be in sats like send, build and listpaymentsincoinjoin where the amount is in sat.
So currently: do paymentincoinjoin of amount 1, then do listpaymentsincoinjoin -> it shows amount 100000000

adamPetho
adamPetho previously approved these changes Nov 24, 2023
Copy link
Collaborator

@adamPetho adamPetho left a comment

Choose a reason for hiding this comment

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

Fought a hard battle, but finally I can write tACK

{
if (payment.State is PendingPayment)
{
_payments.Remove(payment);
Copy link
Collaborator

@adamPetho adamPetho Nov 24, 2023

Choose a reason for hiding this comment

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

I wonder how this works.

I did a payincoinjoin and after that I cancelled it.
It was successfully removed from this list, but the CJ itself will be confirmed once I mine a block. So it's not really cancelled unless the client replaces the CJ with an RBF tx.

So, cancelling payments means only removing the item from the _payments list?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You can only cancel payments that are not still participating in a coinjoin.

Copy link
Collaborator

@adamPetho adamPetho Nov 24, 2023

Choose a reason for hiding this comment

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

Oh OK, so cancelling payments means the GetBestPaymentSet won't select it (bc it's removed from the list) in OutputReg phase.

I think I got it now. Thanks

Copy link
Collaborator

@turbolay turbolay left a comment

Choose a reason for hiding this comment

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

utACK, PR easy to review
There are cf related issues

WalletWasabi/WabiSabi/Client/Batching/PaymentBatch.cs Outdated Show resolved Hide resolved
WalletWasabi/WabiSabi/Client/Batching/PaymentBatch.cs Outdated Show resolved Hide resolved
Copy link
Collaborator

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

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

tack 611c153

Copy link
Collaborator

@yahiheb yahiheb left a comment

Choose a reason for hiding this comment

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

tACK

@yahiheb yahiheb changed the title Add listpayments and cancelpayment RPC call Add listpaymentsincoinjoin and cancelpaymentincoinjoin RPC calls Nov 26, 2023
yahiheb and others added 2 commits November 27, 2023 00:25
Co-authored-by: Turbolay <turbolay@zksnacks.com>
Copy link
Collaborator

@yahiheb yahiheb left a comment

Choose a reason for hiding this comment

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

ok, it's unrelated to this PR: for payincoinjoin if I enter amount 1, it registers 1 BTC, instead of 1 sat.

It registers 1 sat not 1 BTC, so it is working as expected.

There are cf related issues

I fixed them.

tACK

Copy link
Contributor

@AlexisKv AlexisKv left a comment

Choose a reason for hiding this comment

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

Sometimes you don't use arrow function and sometimes you does. Maybe we should follow common styles for short methods (I suggest to use arrow functions, because of looks).

public record Payment(IDestination Destination, Money Amount)
{
public Guid Id { get; } = Guid.NewGuid();
public PaymentState State { get; init; } = new PendingPayment(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

We could avoid using needless null as parameter via using parameterless constructor.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can create a new constructor and move the null there ofc.

@lontivero
Copy link
Collaborator Author

I will merge this PR even when there is a bug because that bug was not introduced here and it is a general bug that affects all the methods that use Money.

@lontivero lontivero merged commit b668f11 into WalletWasabi:master Dec 14, 2023
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants