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

Move withdraw logic to PoolWithdrawManager contract #77

Merged
merged 1 commit into from
Nov 3, 2022
Merged

Move withdraw logic to PoolWithdrawManager contract #77

merged 1 commit into from
Nov 3, 2022

Conversation

venables
Copy link
Contributor

@venables venables commented Nov 2, 2022

  • This leaves a bunch of proxy methods around to make sure we didnt have to modify the content of any tests
  • Some deploy changes were required, so I had to update the deployPool and deployPermissionedPool test support files, which exposed some poorly written tests (some tests incorrectly relied on poolAdmin/Manager being the first record returned from ethers.getSigners()).

Methods from Pool.sol were copied over as-is to the PoolWithdrawManager.sol file. The following were the changes that had to be made:

  • Use of msg.sender was changed to the Pool.sol proxy passing over an owner field, since the Pool was proxying.
  • Burning of fees, and during withdraws still occurs on the Pool contract itself, since the WithdrawManager does not have that capability.

The gains aren't phenomenal, but will be better when we drop the proxy methods:

 ·------------------------------------|--------------|----------------·
 |  Contract Name                     ·  Size (KiB)  ·  Change (KiB)  │
 ·····································|··············|·················
 |  Pool                              ·      15.338  ·        -1.660  │
 ·····································|··············|·················
 |  LoanFactory                       ·      15.564  ·                │
 ·····································|··············|·················
 |  PermissionedPool                  ·      15.756  ·        -1.290  │
 ·····································|··············|·················
 |  PoolFactory                       ·      22.665  ·        -1.225  │
 ·····································|··············|·················
 |  PermissionedPoolFactory           ·      28.702  ·        -0.989  │
 ·------------------------------------|--------------|----------------·

Copy link
Contributor

@ams9198 ams9198 left a comment

Choose a reason for hiding this comment

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

Nice!

@venables venables merged commit 915aaf7 into circlefin:master Nov 3, 2022
@venables venables deleted the venables/val-68-withdraw-contract branch November 3, 2022 19:23
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