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 VRGDA logic into a lib #14

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

0xBeans
Copy link
Contributor

@0xBeans 0xBeans commented Sep 7, 2022

PR for #12

Followed the design of https://github.com/transmissions11/libcompound - split Linear and Logistic into 2 libs.

Copy link

@saucepoint saucepoint left a comment

Choose a reason for hiding this comment

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

I like the idea of immortalizing Linear and Logistic getTargetSaleTime into their own libraries

My initial approach to #12 was only creating a base VRGDALibrary. Wonder if we should do the same here, but I'll let @transmissions11 chime in

from #13

library VRGDALibrary {
    function getVRGDAPrice(int256 _targetPrice, int256 _decayConstant, int256 _timeDelta) internal pure returns (uint256) {
        unchecked {
            // prettier-ignore
            return uint256(wadMul(_targetPrice, wadExp(unsafeWadMul(_decayConstant,
                _timeDelta
            ))));
        }
    }
}

My initial thoughts with this is it becomes kind of verbose:

VRGDALibrary.getVRGDAPrice(..., LinearVRGDA.getTargetSaleTime(...))

Comment on lines +52 to +54
function computeDecayConstant(int256 priceDecayPercent) public pure returns (int256) {
return wadLn(1e18 - priceDecayPercent);
}

Choose a reason for hiding this comment

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

might as well move this to a library, no? it's the same in LogisticVRGDA.sol

@0xBeans 0xBeans closed this Oct 18, 2022
@transmissions11
Copy link
Owner

wait why close?

@transmissions11
Copy link
Owner

sorry my bad for not merging earlier

@0xBeans
Copy link
Contributor Author

0xBeans commented Oct 19, 2022

@transmissions11 was just cleaning up some loose ends on GH, thought maybe this wasn't what you wanted haha Should I reopen?

@0xBeans 0xBeans reopened this Oct 19, 2022
@transmissions11
Copy link
Owner

yes def, sorry i will clean this up asap and get it merged!

@0xBeans
Copy link
Contributor Author

0xBeans commented Oct 19, 2022

yes def, sorry i will clean this up asap and get it merged!

Screen Shot 2022-10-18 at 9 21 50 PM

@nvonpentz
Copy link

Hi @transmissions11 @0xBeans, I'm working on a VRGDA project that needs this library in order to make our contract upgradeable. To that end, I rebased this branch so it is up-to-date with current master branch, except for the LogisticToLinear implementation since it was only added recently. I pushed that branch here https://github.com/transmissions11/VRGDAs/compare/master...nvonpentz:VRGDAs:library-rebase?expand=1.

When making the remaining changes to the LogisticToLinear implementation, I noticed the abstractions didn't fit as well, so I created a fresh branch with a different approach. You can see that here https://github.com/transmissions11/VRGDAs/compare/master...nvonpentz:VRGDAs:library?expand=1. The main differences are that it

  • Creates one library instead of two, with only one getVRGDAPrice function (similar to master), and separate getTargetSaleTimeLinear and getTargetSaleTimeLogistic functions.
  • Does not remove all the abstract classes. Instead, logic common between the different VRGDA types (i.e. the getVRGDAPrice function) is defined in an abstract VRDGA class that Linear, Logitistic, and LogisticToLinear inherit from. I implemented things such that the interfaces defined by the current abstract contracts on master are preserved.

If it's useful, I'm happy to open a PR for either, but I also want to be respectful of the work @0xBeans has done already. In this case I went ahead and implemented in order to unblock myself. Cheers!

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.

4 participants