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 PoolInfoUtilsMulticall support in updatePool #77

Merged
merged 22 commits into from
Dec 20, 2023

Conversation

MikeHathaway
Copy link
Contributor

@MikeHathaway MikeHathaway commented Oct 26, 2023

@MikeHathaway MikeHathaway marked this pull request as ready for review October 28, 2023 20:51
@MikeHathaway MikeHathaway changed the title Info multicall v2 Add PoolInfoUtilsMulticall support in updatePool Oct 28, 2023
* add poolBalanceDetails to updatePool

* update tests for usage of poolBalanceDetails

* fix remaining tests

---------

Co-authored-by: Mike <mikehathaway@makerdao.com>
@MikeHathaway MikeHathaway changed the base branch from rc8-updates to develop October 30, 2023 22:06
Copy link
Contributor

@EdNoepel EdNoepel left a comment

Choose a reason for hiding this comment

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

Some comments inline.

I thought we were calling calculateLendRate from places other than updatePool, so was surprised to see no impact from other methods. I can test deployment on ushas when time permits.

@@ -34,6 +34,9 @@ poolInfoUtilsAddressTable.set('matic', Address.fromString('0xA9Ada58DD3c820b30D3
poolInfoUtilsAddressTable.set('goerli', Address.fromString('0x08F304cBeA7FAF48C93C27ae1305E220913a571d'))
poolInfoUtilsAddressTable.set('mumbai', Address.fromString('0x39250241CC84Dadb1cDFE3A1a717631e2aA603eB'))
poolInfoUtilsAddressTable.set('ganache', Address.fromString('0xab56A77bDFe82b36875e92CE717fE533C1709A9D'))
export const poolInfoUtilsMulticallAddressTable = new TypedMap<string, Address>()
poolInfoUtilsMulticallAddressTable.set('goerli', Address.fromString('0x12874db433dBF1D0f3c73B39F96B009093A56E0E'))
Copy link
Contributor

Choose a reason for hiding this comment

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

That's the address for the old contract, before your most recent changes to the PR. The new one is at 0x49B8AAd62e1BBBA1A69CA8B3E24383F29828F718.

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 to most recent deployment address

mockGetPoolPricesInfo(pool, expectedPoolPricesInfo)

const expectedRatesAndFeesInfo = new RatesAndFees(
// FIXME: set correct lim value here
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 FIXME need to be addressed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed, we're already checking values are being set and this would be a duplicate and more brittle mirror of the contract unit tests

pool.quoteTokenBalance = wadToDecimal(poolBalanceDetails.quoteTokenBalance)
// FIXME: If isNFT then don't convert wadToDecimal?
pool.collateralBalance = wadToDecimal(poolBalanceDetails.collateralTokenBalance)
// FIXME: update t0debt -> need to take into account pending debt?
Copy link
Contributor

Choose a reason for hiding this comment

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

No, t0debt, by design, omits pending debt, which updates every block. To get pool debt, caller must query pending inflator directly from contract, and multiply by this t0debt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed dead comments

Copy link
Contributor

@EdNoepel EdNoepel left a comment

Choose a reason for hiding this comment

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

Would like to test this on ushas.

src/utils/constants.ts Outdated Show resolved Hide resolved
MikeHathaway and others added 3 commits December 20, 2023 15:15
* wip rc9 updates

* update abis

* update borrowerInfo interface; update comments and tests

* fix test issues

* add latest rc9 updates; fix tests

---------

Co-authored-by: Mike <mikehathaway@makerdao.com>
Copy link
Contributor

@EdNoepel EdNoepel left a comment

Choose a reason for hiding this comment

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

Updated addresses and ABIs. Was able to deploy to dev server pointing at today's Goerli deployment and query an empty pool.

@MikeHathaway MikeHathaway merged commit 4727bbd into develop Dec 20, 2023
1 check passed
@MikeHathaway MikeHathaway deleted the info-multicall-v2 branch December 20, 2023 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants