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

Deprecating event liquidity #1697

Closed
4 tasks done
sabineschaller opened this issue Aug 2, 2023 · 4 comments · Fixed by #2318
Closed
4 tasks done

Deprecating event liquidity #1697

sabineschaller opened this issue Aug 2, 2023 · 4 comments · Fixed by #2318
Labels
story Major feature that can be broken up into smaller tasks

Comments

@sabineschaller
Copy link
Member

sabineschaller commented Aug 2, 2023

Context

Currently, we deposit and withdraw liquidity as follows:

  • asset liquidity -> based on asset id
  • peer liquidity -> based on peer id
  • payment pointer l. -> based on payment pointer id
  • incoming payment l. -> based on event id
  • outgoing payment l. -> based on event id

To standardize, we want to also allow depositing and withdrawing liquidity by payment id. We want to deprecate eventId-based liquidity management.

Todos

Related

@github-project-automation github-project-automation bot moved this to Backlog in Rafiki Aug 2, 2023
@sabineschaller sabineschaller added the story Major feature that can be broken up into smaller tasks label Aug 2, 2023
@sabineschaller sabineschaller changed the title Removing event liquidity Deprecating event liquidity Aug 2, 2023
@mkurapov mkurapov added this to the v1.0.0-alpha.4 milestone Aug 3, 2023
@BlairCurrey
Copy link
Contributor

BlairCurrey commented Oct 31, 2023

@sabineschaller @mkurapov

The withdrawEventLiquidity mutation tests include a case for the wallet_address.web_monetization event which got me wondering - are we currently or can we handle that without withdrawEventLiquidity? As far as I can see this is not handled in the MASE webhook server despite the the integration docs saying it “should withdraw that liquidity from the wallet address and credit the receiver’s account”. Looks like maybe implementing that handler in the MASE using createWalletAddressWithdrawal should be an additional task on this story?

@mkurapov
Copy link
Contributor

Looks like maybe implementing that handler in the MASE using createWalletAddressWithdrawal should be an additional task on this story?

I think that makes sense, however, I believe in order to receive the wallet_address.web_monetization event we need to call the triggerWalletAddressEvents admin API which we are not doing from the MASE. Do you think it's worth adding the wallet_address.web_monetization handler if we don't have anything in place for triggerWalletAddressEvents yet?

Speaking of triggerWalletAddressEvents, maybe it's worth renaming that as well?

@BlairCurrey
Copy link
Contributor

BlairCurrey commented Oct 31, 2023

After speaking with Max we concluded a few things:

  • currently withdrawEventLiquidity creates a withdrawal using the event id which ensures duplicate transfers for the same event are not created. We concluded that we can look this up internally in the new endpoints (as opposed to passing it in, or using some other id). For example, in the new withdrawIncomingPaymentLiquidity resolver we can get the latest webhook event where withdrawalAccountId is incomingPayment.id and type is either incoming_payment.completed or incoming_payment.expired. And similar for the other new endpoints
  • we can implement a handleWalletAddressWebMonetization that calls createWalletAddressWithdrawal and test that manually and leave the MASE triggerWalletAddressEvents call out of scope due to complexity. I made an issue for that which includes more details on how to manually test. already a task here Include handler for payment_pointer.web_monetization webhook event in Mock ASE #2057. I closed the duplicate task but it includes some test instructions that might still be useful: Add handleWalletAddressWebMonetization to ASE #2150

@BlairCurrey
Copy link
Contributor

Branch off bc/1697/deprecate-event-liquidity for these story issues. Will collect all the changes there before merging into main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
story Major feature that can be broken up into smaller tasks
Projects
Status: Q4 2023
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants