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 some new read-only functions to market and pool #2337

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dbeal-eth
Copy link
Contributor

@dbeal-eth dbeal-eth commented Nov 19, 2024

should make it much easier to understand the underlying computations that go behind capacity locked or similar issues in the future

also fixes credit capacity locking with deposited collateral issue.

should make it much easier to understand the underlying computations
that go behind capacity locked or similar issues in the future
Copy link
Contributor

@noisekit noisekit left a comment

Choose a reason for hiding this comment

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

can we have some tests maybe?

@dbeal-eth
Copy link
Contributor Author

can we have some tests maybe?

they are forthcoming tomorrow, now that fix is in hand

@@ -298,7 +298,14 @@ library Market {
*
*/
function isCapacityLocked(Data storage self) internal view returns (bool) {
return self.creditCapacityD18 < getLockedCreditCapacity(self).toInt();
(
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this update mean isCapacityLocked now requires offchain price update?

Copy link
Contributor

Choose a reason for hiding this comment

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

As this is an internal function we need to check which public methods get affected and start requiring offchain price updates attached. May break a lot of integrations.

Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend removing the fix from add some new read-only functions to market and pool PR and making another one specifically for this fix.

Copy link
Contributor

@noisekit noisekit left a comment

Choose a reason for hiding this comment

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

  1. Tests
  2. Removing the fix from this pr and submit it separately
  3. For the fix - list all the affected public methods that will flip from not requiring offhain price update

@dbeal-eth
Copy link
Contributor Author

  1. Tests

    1. Removing the fix from this pr and submit it separately

    2. For the fix - list all the affected public methods that will flip from not requiring offhain price update

  1. its coming
  2. will do
  3. will also list in other PR but the list is:
  • VaultModule.delegateCollateral: function already depends on the same offchain prices prior to capacity locked check
  • PoolModule.setPoolConfiguration: same as `delegateCollateral
  • MarketManagerModule.isMarketCapacityLocked: as far as I know nobody actually uses this view function on an actual app, but even if they do our docs say that any method could start requiring erc7412 at any time right? lol

@noisekit
Copy link
Contributor

  • MarketManagerModule.isMarketCapacityLocked

Used in liquidity ui

@dbeal-eth
Copy link
Contributor Author

  • MarketManagerModule.isMarketCapacityLocked

Used in liquidity ui

lol ok well guess we need to add erc7412 to the liquidity ui for that call (really should be wrapped around all calls imo)

@noisekit
Copy link
Contributor

No

really should be wrapped around all calls imo

Bad idea b cause that call is much heavier and harder to debug than plain call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants