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

Refactor open interest and oi shares reduction #120

Merged
merged 12 commits into from
Jan 18, 2024
Merged

Conversation

magnetto90
Copy link
Contributor

@magnetto90 magnetto90 commented Jan 2, 2024

liquidate() and unwind() functions share a considerable amount of code. This PR creates an internal function to avoid these duplicates. Thanks Uncle Bob.

Merge after #133

Copy link
Member

@TomasCImach TomasCImach left a comment

Choose a reason for hiding this comment

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

Consider removing oiTotalOnSide and oiTotalSharesOnSide from the function parameters and fetching these values with:

uint256 oiTotalOnSide = pos.isLong ? oiLong : oiShort;
uint256 oiTotalSharesOnSide = pos.isLong ? oiLongShares : oiShortShares;

or can be made with:

        if (pos.isLong) {
            oiLong = oiLong.subFloor(
                pos.oiCurrent(fraction, oiLong, oiLongShares)
            );
            oiLongShares -= pos.oiSharesCurrent(fraction);
        } else {
            oiShort = oiShort.subFloor(
                pos.oiCurrent(fraction, oiShort, oiShortShares)
            );
            oiShortShares -= pos.oiSharesCurrent(fraction);
        }

EperezOk
EperezOk previously approved these changes Jan 4, 2024
@magnetto90 magnetto90 changed the base branch from main to staging January 11, 2024 16:22
@magnetto90 magnetto90 dismissed EperezOk’s stale review January 11, 2024 16:22

The base branch was changed.

Copy link
Member

@TomasCImach TomasCImach left a comment

Choose a reason for hiding this comment

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

pending tests

@magnetto90
Copy link
Contributor Author

pending tests

Used brownie test for this PR since its a refactor and not a new feature.

TomasCImach
TomasCImach previously approved these changes Jan 18, 2024
@magnetto90 magnetto90 dismissed TomasCImach’s stale review January 18, 2024 11:45

The merge-base changed after approval.

@TomasCImach TomasCImach merged commit db84c44 into staging Jan 18, 2024
2 checks passed
@TomasCImach TomasCImach deleted the liquidate-refactor branch January 18, 2024 15:08
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.

3 participants