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

Pool maintanence #663

Merged
merged 5 commits into from
Mar 7, 2023
Merged

Pool maintanence #663

merged 5 commits into from
Mar 7, 2023

Conversation

grandizzy
Copy link
Contributor

  • Consistent naming of LPs management functions and events
  • Reorg interface functions and events based on their action
  • Move LPs transfer logic from Pool to external LenderAction library

contract size before change

============ Deployment Bytecode Sizes ============
  ERC721Pool               -  24,393B  (99.25%)
  ERC20Pool                -  23,876B  (97.15%)
  Auctions                 -  20,575B  (83.72%)

after change

============ Deployment Bytecode Sizes ============
  ERC721Pool               -  24,121B  (98.14%)
  ERC20Pool                -  23,604B  (96.04%)
  Auctions                 -  20,575B  (83.72%)

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.

Just a comment regarding a comment.

/**
* @notice Called by lenders to update pool interest rate (can be updated only once in a 12 hours period of time).
*/
function updateInterest() external;
Copy link
Contributor

Choose a reason for hiding this comment

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

Unsure why this belongs in lender actions. Lender is incentivized to call when rate would go up, borrower is incentivized to call when rate would go down. Understand it needs to live somewhere. Maybe just update comment to remove by lenders?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, renamed to by actors with 2512e5d (ideally we would have a separate InterestRate interface)

@grandizzy grandizzy mentioned this pull request Mar 6, 2023
@grandizzy grandizzy requested a review from dmitriia March 6, 2023 17:48
* @param owner LPs owner.
* @param spender Address approved to transfer LPs.
* @param indexes Bucket indexes of LPs approved.
* @param amounts LP amounts added (ordered by approved indexes).
Copy link

Choose a reason for hiding this comment

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

Here it should be 'LP amounts removed'

* @dev Intended for use by the PositionManager contract.
* @param spender The new owner of the LPs.
* @param indexes Bucket indexes from where LPs are transferred.
* @param amounts The amounts of LPs approved to transfer.
Copy link

Choose a reason for hiding this comment

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

Similarly 'disapproved' here

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 in 70fc3b7

Copy link

@dmitriia dmitriia left a comment

Choose a reason for hiding this comment

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

Looks ok

* @dev write state:
* - increment LPs allowances
* @dev emit events:
* - SetLPsAllowance
Copy link

Choose a reason for hiding this comment

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

'IncreaseLPsAllowance' here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, updated in c9fcfdc

* @dev write state:
* - decrement LPs allowances
* @dev emit events:
* - SetLPsAllowance
Copy link

Choose a reason for hiding this comment

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

'DecreaseLPsAllowance' here

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 in c9fcfdc

* @dev write state:
* - decrement LPs allowances
* @dev emit events:
* - SetLPsAllowance
Copy link

Choose a reason for hiding this comment

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

'RevokeLPsAllowance'

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 in c9fcfdc

Copy link

@dmitriia dmitriia left a comment

Choose a reason for hiding this comment

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

Looks ok

Copy link
Collaborator

@MikeHathaway MikeHathaway left a comment

Choose a reason for hiding this comment

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

LGTM

@grandizzy grandizzy merged commit 99a5f6a into develop Mar 7, 2023
@grandizzy grandizzy deleted the pool-maintanence branch March 7, 2023 04:00
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.

4 participants