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.

Sign up for GitHub

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

Jump to bottom

feat: asset mgmt base class #75

Merged
merged 12 commits into from
Sep 11, 2022
Merged

feat: asset mgmt base class #75

merged 12 commits into from
Sep 11, 2022

Conversation

xenide
Copy link
Contributor

@xenide xenide commented Sep 7, 2022 •
edited
Loading

Motivation

  • abstract asset management logic into a base class to avoid repetition and drift

Solution

  • please see my approach in AssetManagedPairTest::testSetManager for the table test. If this approach is okay, I will implement it for all tests in the file
  • there might still be 1/2 tests that work on specific curves, for example testManageReserves_KStillHolds and testSync as the StablePair doesn't have a sync function (yet)
  • for IAssetManager::adjustManagement(), is there. reason we are using int224 in the mock implementation instead of int256 or int112?

@xenide xenide linked an issue Sep 7, 2022 that may be closed by this pull request
refactor(pair): move asset management functions into a shared base class #73
Closed
OliverNChalk
OliverNChalk requested changes Sep 7, 2022
View reviewed changes
src/asset-management/AssetManagedPair.sol Outdated Show resolved Hide resolved
src/asset-management/AssetManagedPair.sol Outdated Show resolved Hide resolved
src/asset-management/AssetManagedPair.sol Outdated Show resolved Hide resolved
src/curve/stable/StablePair.sol Show resolved Hide resolved
src/interfaces/IAssetManagedPair.sol Outdated Show resolved Hide resolved
src/interfaces/IAssetManager.sol Outdated Show resolved Hide resolved
test/unit/AssetManagedPair.t.sol Outdated Show resolved Hide resolved
Base automatically changed from feat/am-stablepair to dev September 10, 2022 23:33
@xenide xenide merged commit 3573b30 into dev Sep 11, 2022
@xenide xenide deleted the feat/asset-mgmt-base-class branch September 11, 2022 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OliverNChalk OliverNChalk OliverNChalk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

refactor(pair): move asset management functions into a shared base class
2 participants
@xenide @OliverNChalk