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

feat: rename add liquidity mutations to deposit #2304

Conversation

BlairCurrey
Copy link
Contributor

@BlairCurrey BlairCurrey commented Jan 3, 2024

Changes proposed in this pull request

Renamed the following and updated related names, frontend, postman, docs for:

  • addAssetLiquidity mutation -> depositAssetLiquidity
  • addPeerLiquidity mutation -> depositPeerLiquidity
  • addedLiquidity field on CreateOrUpdatePeerByUrlInput mutation -> depositedPeerLiquidity

Context

fixes #2303

Checklist

  • Related issues linked using fixes #number
  • Tests added/updated
  • Documentation added
  • Make sure that all checks pass
  • Postman collection updated

BREAKING CHANGE: Renamed addAssetLiquidity resolver to depositAssetLiquidity for clarity and consistency.
Copy link

netlify bot commented Jan 3, 2024

Deploy Preview for brilliant-pasca-3e80ec ready!

Name Link
🔨 Latest commit
🔍 Latest deploy log https://app.netlify.com/sites/brilliant-pasca-3e80ec/deploys/659c564e59b43b1b9493a733
😎 Deploy Preview https://deploy-preview-2304--brilliant-pasca-3e80ec.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@github-actions github-actions bot added type: tests Testing related pkg: backend Changes in the backend package. pkg: frontend Changes in the frontend package. type: source Changes business logic pkg: mock-ase pkg: documentation Changes in the documentation package. type: documentation (archived) Improvements or additions to documentation labels Jan 3, 2024
@BlairCurrey BlairCurrey changed the base branch from main to bc/1697/deprecate-event-liquidity January 3, 2024 16:56
@github-actions github-actions bot removed pkg: documentation Changes in the documentation package. type: documentation (archived) Improvements or additions to documentation labels Jan 3, 2024
@github-actions github-actions bot added pkg: documentation Changes in the documentation package. type: documentation (archived) Improvements or additions to documentation labels Jan 3, 2024
@BlairCurrey BlairCurrey marked this pull request as ready for review January 4, 2024 19:01
@JoblersTune
Copy link
Collaborator

I found more instances of 'add'

  • packages/backend/src/payment-method/ilp/auto-peering/errors.ts: [AutoPeeringError.LiquidityError]: 'Could not add liquidity to peer'
  • packages/backend/src/payment-method/ilp/auto-peering/service.test.ts: returns error if could not add liquidity during peer creation and returns error if could not add liquidity during peer update
  • packages/backend/src/payment-method/ilp/auto-peering/service.ts: Could not add liquidity to peer
  • packages/backend/src/payment-method/ilp/peer/service.test.ts: Add Liquidity and Can add liquidity to peer
  • packages/backend/src/payment-method/ilp/peer/service.ts: error trying to add initial liquidity

Copy link
Member

@sabineschaller sabineschaller left a comment

Choose a reason for hiding this comment

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

I could not find any more instances of "add" but one test failed for me locally:
packages/backend/src/graphql/middleware/index.test.ts
● GraphQL Middleware › lockGraphQLMutationMiddleware › throws error on concurrent call with same key

@BlairCurrey
Copy link
Contributor Author

I could not find any more instances of "add" but one test failed for me locally: packages/backend/src/graphql/middleware/index.test.ts ● GraphQL Middleware › lockGraphQLMutationMiddleware › throws error on concurrent call with same key

@sabineschaller I could not reproduce this and it's passing here. Perhaps an intermittent failure, or resolved after pnpm clean && pnpm install? I'm not seeing anything related to the changes I've made in that test.

@mkurapov
Copy link
Contributor

mkurapov commented Jan 8, 2024

I'm trying to figure out why the deploy preview https://deploy-preview-2304--brilliant-pasca-3e80ec.netlify.app/apis/backend/mutations/#addpeerliquidity still contains the addPeerLiquidity mutation, even though locally it looks ok

I'm guessing it's because its not being merged into main. Retriggered and looks good.

Comment on lines 313 to 314
"Initial amount of liquidity to deposit for peer"
depositedLiquidity: UInt64
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Initial amount of liquidity to deposit for peer"
depositedLiquidity: UInt64
"Amount of liquidity to deposit for peer"
liquidityToDeposit: UInt64

Maybe liquidityToDeposit reads better? let me know what you think.

Also, I updated the original description I wrote, since its not just the initial time that liquidity gets added but every time this mutation is called (ie. this liquidity gets added on create and update)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, can you update the Auto-peering section (example mutation) in the docs to reflect this change please

Copy link
Contributor Author

@BlairCurrey BlairCurrey Jan 8, 2024

Choose a reason for hiding this comment

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

Do we want to differentiate this from the CreatePeerInput.initialLiquidity field? Before your edit here the comment description was identical, although the name was different. Just noticing that we could call this initialLiquidity as well but perhaps this doesn't make sense in the context of updating, hence the comment update and having a different name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated d708feb

Copy link
Contributor

@mkurapov mkurapov Jan 8, 2024

Choose a reason for hiding this comment

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

It's ok that they are different, since the behaviour isn't the same. Only other change would be to (maybe?) change initialLiquidity to initialLiquidityToDeposit but I think it's already self explanatory (+ the comment is there as well)

@@ -125,7 +125,7 @@ Query Variables (substitute the asset ID from the "create asset" response for `I
"outgoing": {"endpoint": "ilp.othergreatwallet.com", "authToken": "theirtoken"}
},
"assetId": "INSERT_ASSET_ID",
"initialLiquidity: <optionally, and intial amount of liquity to provision. Liquidity can also be added via the `AddPeerLiquidity` mutation described below>
"initialLiquidity: <optionally, and intial amount of liquity to provision. Liquidity can also be deposited via the `DepositPeerLiquidity` mutation described below>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you are missing a closing quote around "initialLiquidity(")

@sabineschaller
Copy link
Member

I could not reproduce this and it's passing here. Perhaps an intermittent failure, or resolved after pnpm clean && pnpm install? I'm not seeing anything related to the changes I've made in that test.

@BlairCurrey I tried that (again after upgrading pnpm) and still the same issue. But it works in the CI so it must be a me-problem.

@BlairCurrey BlairCurrey merged commit 41e31ef into bc/1697/deprecate-event-liquidity Jan 9, 2024
15 checks passed
@BlairCurrey BlairCurrey deleted the bc/2303/rename-add-liquidity-to-deposit branch January 9, 2024 17:02
@sabineschaller sabineschaller added the breaking Issue/PR that introduces breaking changes label Jan 10, 2024
BlairCurrey added a commit that referenced this pull request Jan 10, 2024
* feat(backend): add liquidity to payment and wallet address resolvers (#2216)

* feat(backend): add liquidity to payment and wallet address resolvers

* feat: add liquidity test to wallet address

* feat: add more liquidity tests to payments

* chore: improve wording on test

* feat: add payment liquidity endpoints (#2151)

* feat(backend); add payment liquidity resolvers to schema

* feat(backend): stub in payment liquidity resolvers

* chore(backend): cleanup test

* feat(backend): implement withdrawIncomingPaymentLiquidity

* refactor(localenv): use withdrawIncomingPaymentLiquidity

* feat(postman): add withdrawIncomingPaymentLiquidity request

* chore(backend): add comments clarifying webhook event model

* feat(backend): add wh method

add getLatestByAccountIdAndTypes to be used in new liquidity resolvers

* feat(backend): use getLatestByAccountIdAndTypes

* chore(documentation): fix typo

* feat(backend): implement withdrawOutgoingPaymentLiquidity

* feat(postman): add withdrawOutgoingPaymentLiquidity gql request

* fix(backend): linter

* fix(backend): linter

* fix(backend): withdrawIncomingPaymentLiquidity tests

* test(backend): all event types for withdrawIncomingPaymentLiquidity

* feat(backend): WIP withdrawOutgoingPaymentLiquidity tests

* chore(backend): better error response

* test(backend): withdrawOutgoingPaymentLiquidity error cases

* feat(postman): new depositOutgoingPaymentLiquidity request

* feat(backend): DepositOutgoingPaymentLiquidity resolver

* test(backend): depositOutgoingPaymentLiquidity

* feat: update mock ASE resolvers

* refactor: change check to deposit event

* refactor(documentation): update docs

* fix(backend): lint error

* fix(backend): usage of query first method

* fix(backend): add error msg

* feat(documentation): idempotency explanation

* fix(backend): bad commit 44fc2dd

* feat(backend): mark event liquidity resolvers deprecated

* refactor(backend): webhook table related resource

* fix(documenation): typo

* chore(backend): format,linting

* fix(documentation): improve idempotency doc

* Update packages/backend/src/webhook/model.ts

Co-authored-by: Max Kurapov <[email protected]>

* feat(backend): relate peer, asset to webhookEvents

* feat(backend): improve constraint, update existing

* fix(backend): fkey relation typo

* fix(backend): create webhook event test util

* feat(backend): improve webhook related resource checks

* fix(backend): use AssetEvent model

* chore(backend): fix lint error

* fix(backend): remove no longer used code

* fix(backend): lint error

* refactor(backend): rename payment to outgoingpayment

* fix(backend): eslint comment

* fix(backend): tests

* fix(backend): test

* refactor(backend): drop constraint

* chore(backend): remove unused fn

---------

Co-authored-by: Max Kurapov <[email protected]>

* fix(backend): use updated config

* feat(frontend): payment liquidity admin UI (#2290)

* feat(backend); add payment liquidity resolvers to schema

* feat(backend): stub in payment liquidity resolvers

* chore(backend): cleanup test

* feat(backend): implement withdrawIncomingPaymentLiquidity

* refactor(localenv): use withdrawIncomingPaymentLiquidity

* feat(postman): add withdrawIncomingPaymentLiquidity request

* chore(backend): add comments clarifying webhook event model

* feat(backend): add wh method

add getLatestByAccountIdAndTypes to be used in new liquidity resolvers

* feat(backend): use getLatestByAccountIdAndTypes

* chore(documentation): fix typo

* feat(backend): implement withdrawOutgoingPaymentLiquidity

* feat(postman): add withdrawOutgoingPaymentLiquidity gql request

* fix(backend): linter

* fix(backend): linter

* fix(backend): withdrawIncomingPaymentLiquidity tests

* test(backend): all event types for withdrawIncomingPaymentLiquidity

* feat(backend): WIP withdrawOutgoingPaymentLiquidity tests

* chore(backend): better error response

* test(backend): withdrawOutgoingPaymentLiquidity error cases

* feat(postman): new depositOutgoingPaymentLiquidity request

* feat(backend): DepositOutgoingPaymentLiquidity resolver

* test(backend): depositOutgoingPaymentLiquidity

* feat: update mock ASE resolvers

* refactor: change check to deposit event

* refactor(documentation): update docs

* fix(backend): lint error

* fix(backend): usage of query first method

* fix(backend): add error msg

* feat(documentation): idempotency explanation

* fix(backend): bad commit 44fc2dd

* feat(backend): mark event liquidity resolvers deprecated

* refactor(backend): webhook table related resource

* fix(documenation): typo

* chore(backend): format,linting

* fix(documentation): improve idempotency doc

* Update packages/backend/src/webhook/model.ts

Co-authored-by: Max Kurapov <[email protected]>

* feat(backend): relate peer, asset to webhookEvents

* feat(backend): improve constraint, update existing

* fix(backend): fkey relation typo

* fix(backend): create webhook event test util

* feat(backend): improve webhook related resource checks

* fix(backend): use AssetEvent model

* chore(backend): fix lint error

* fix(backend): remove no longer used code

* fix(backend): lint error

* refactor(backend): rename payment to outgoingpayment

* fix(backend): eslint comment

* fix(backend): tests

* fix(backend): test

* feat(frontend): basic payments page w/ filter

* refactor(backend): drop constraint

* refactor(frontend): use exisitng util

* feat(frontend): initial incoming payment page

* chore(backend): remove unused fn

* feat(frontend): link to wallet address in payment page

* feat(frontend): add incoming payment liquidity section

* feat: add incoming payment liquidity and outgoing payment page

* feat: generate types

* fix(frontend): lint errors

* refactor: decouple badge from walletaddresstatus

* fix: badgeColorByStatus type

* refactor: type

* feat: payment state badges, improve payment page layout

* feat(frontend): conditionally disable withdraw button

* refactor(frontend): payment amount fields

* feat(frontend): implement deposit/withdraw liquidity

* refactor: clarify hidden input

* refactor: simplify liquidity confirm dialog

* feat(frontend): add wallet address liquidity

* feat(frontend): prettify json

* chore(frontend): rm unused fields

* chore(frontend): format

* chore: format

* Update packages/frontend/app/routes/payments.outgoing.$outgoingPaymentId.tsx

Co-authored-by: Timea <[email protected]>

* Update packages/frontend/README.md

Co-authored-by: Timea <[email protected]>

* feat(frontend): change 'add' liquidity to 'deposit'

* fix(frontend): corrected amount shown

#2290 (comment)

* fix(frontend): renamed file. removed extra period.

* refactor(frontend): moved config objects outside pages

avoids unnecessarily recreating the object every time the page is rendered.

* refactor: rename variable for clarity

* refactor(frontend): use detail element to avoid usestate

* fix(frontend): change em elements to i

* feat: move payment state into field group

* feat(frontend): change field button to link

* chore(frontend): remove unused styling & props

reverts 20f9583

* feat(frontend): add date column to payments page

* fix(frontend): withdraw button doesnt show for non-completed

fixes issues described here for non completed incoming payments #2290 (review)

* chore: format

* fix(backend): use new env var

* Update packages/frontend/app/routes/payments._index.tsx

Co-authored-by: Sarah Jones <[email protected]>

* fix(frontend): move date toLocaleString to client side

* feat(frontend): add date col to webhooks page

* feat(frontend): add display amount to liquidity confirm dialog

---------

Co-authored-by: Max Kurapov <[email protected]>
Co-authored-by: Timea <[email protected]>
Co-authored-by: Sarah Jones <[email protected]>

* feat: rename add liquidity mutations to deposit (#2304)

* feat(liquidity): rename addAssetLiquidity to depositAssetLiquidity

BREAKING CHANGE: Renamed addAssetLiquidity resolver to depositAssetLiquidity for clarity and consistency.

* feat(liquidity): rename addPeertLiquidity to depositPeerLiquidity

BREAKING CHANGE: Renamed addPeerLiquidity resolver to depositPeerLiquidity for clarity and consistency.

* chore(backend): update test name verbiage from add to deposit

* chore(backend): update error verbiage from add to deposit

* chore(backend): update gql schema comment verbiage from add to deposit

* fix(frontend): upadte to use new resolver names

* fix(localenv): update mock ase to use new mutation names

* fix(localenv,documentation): update docs

* refactor: rename add liquidity service methods to deposit

* feat: rename addedLiquidity to depositedLiquidity

BREAKING CHANGE: Renamed addedLiquidity field to depositedLiquidity on graphql  CreateOrUpdatePeerByUrlInput for consistency.

* fix(postman): update request to use new deposit name

* fix(frontend): change "add" to "deposit" liquidity

* refactor(backend): update verbiage from add to deposit liquidity

* refactor: change depositedLiquidity to liquidityToDeposit

* Update packages/documentation/src/content/docs/concepts/interledger-protocol/peering.md

Co-authored-by: Max Kurapov <[email protected]>

* fix(documentation): bad json

---------

Co-authored-by: Max Kurapov <[email protected]>

* fix(frontend): move toLocale date conversion to client from server

* fix(frontend): deposit outgoing payment liquidity display amount

* chore: format

* fix(frontend): field display (Received -> Receive)

---------

Co-authored-by: Nathan Lie <[email protected]>
Co-authored-by: Max Kurapov <[email protected]>
Co-authored-by: Timea <[email protected]>
Co-authored-by: Sarah Jones <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Issue/PR that introduces breaking changes pkg: backend Changes in the backend package. pkg: documentation Changes in the documentation package. pkg: frontend Changes in the frontend package. pkg: mock-ase type: documentation (archived) Improvements or additions to documentation type: source Changes business logic type: tests Testing related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename liquidity resolvers to use "deposit" instead of "add."
5 participants