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

Fixed veOgv bug on dApp #371

Merged
merged 2 commits into from
Feb 2, 2023
Merged

Fixed veOgv bug on dApp #371

merged 2 commits into from
Feb 2, 2023

Conversation

HrikB
Copy link
Contributor

@HrikB HrikB commented Feb 2, 2023

My diagnosis of this bug was two fold.

The first is that the code for calculating the veOgv in the smart contract is quite different from the one used in the dApp.

OgvStaking.sol

function previewPoints(uint256 amount, uint256 duration)
        public
        view
        returns (uint256, uint256)
    {
        // Basically always block.timestamp
        uint256 start = block.timestamp > epoch ? block.timestamp : epoch;
        uint256 end = start + duration;
        uint256 endYearpoc = ((end - epoch) * 1e18) / 365 days;
        uint256 multiplier = PRBMathUD60x18.pow(YEAR_BASE, endYearpoc);
        return ((amount * multiplier) / 1e18, end);
    }

LockupForm.tsx

const veOgvFromOgvLockup =
    lockupAmount * votingDecayFactor ** (lockupDuration / 12);

The typescript version yields quite a similar output but it has a lot of rounding errors that often would go unnoticed because it's not huge; but often wouldn't. So changed that to follow the smart contract's calculation method.

The second and bigger issue (and the one that was the crux of the bug @micahalcorn found) came from a use of a static block.timestamp. The blockTimestamp used in the dApp is one that is fetched in the initial page load and then never updated. This is a problem because the block.timestamp is part of the calculation for the output veOgv quantity.

If a user were to go on the page, leave for an hour and come back to confirm their transaction... they would find a very different veOgv output quantity.

The change made here is to wrap an already existing block timestamp fetching hook in a useInterval() delimited at 4s. At an average 15s block production time, this is more than enough to ensure a non-stale timestamp.

One thing to note is the formula in dApp will use the current block timestamp as opposed to the next (the one in which the user's transaction will be confirmed). However, the difference in output is negligible.

Closes #342

@HrikB HrikB self-assigned this Feb 2, 2023
blockTimestamp,
intervalId = setInterval(() => {
Promise.all([getBlockTimestamp()]).then(([blockTimestamp]) => {
console.log(blockTimestamp);
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: can remove these logs before merging

@@ -145,8 +145,15 @@ const LockupForm: FunctionComponent<LockupFormProps> = ({ existingLockup }) => {
// as specified here: https://github.com/OriginProtocol/ousd-governance/blob/master/contracts/OgvStaking.sol#L21
const votingDecayFactor = 1.8;

const veOgvFromOgvLockup =
lockupAmount * votingDecayFactor ** (lockupDuration / 12);
const epoch = 1657584000;
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: A comment on where this timestamp came from would be helpful

@HrikB HrikB merged commit 1474417 into master Feb 2, 2023
@HrikB HrikB deleted the veogv-bug branch February 2, 2023 17:16
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.

Unexpected veOGV amount
2 participants