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

fix: deposit flow #103

Merged
merged 5 commits into from
Sep 18, 2024
Merged

fix: deposit flow #103

merged 5 commits into from
Sep 18, 2024

Conversation

Schlagonia
Copy link
Collaborator

@Schlagonia Schlagonia commented Sep 10, 2024

Description

Still allow deposits to work if totalSupply == 0 but totalAssets > 0. Just make PPS == 1.

This can happen due to the profit unlocking if all shares are redeemed while some profit is still unlocking, at the end of the unlock time totalAssets >0 but totalSupply == 0.

  • Still return 0 in convertToShares if totalSupply > 0 but totalAssets == 0. Meaning a full loss.

  • Optimize to not cache totalAssets_ until needed

  • Add the check for share receiver to the maxDeposit/maxMint checks instead. So that the view functions are more accurate.

  • Update the variable name from owner => receiver to be more accurate
    Fixes # (issue)

Checklist

  • I have run solidity linting
  • I have run the tests on my machine
  • I have followed commitlint guidelines
  • I have rebased my changes to the latest version of the main branch
  • I have updated the SPECIFICATION.md for any relevant changes

@Schlagonia Schlagonia changed the title fix: conversion fix: deposit flow Sep 12, 2024
Copy link
Collaborator

@fp-crypto fp-crypto left a comment

Choose a reason for hiding this comment

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

LGTM, but probably should be gated on yearn/yearn-vaults-v3#209

Copy link
Collaborator

@wavey0x wavey0x left a comment

Choose a reason for hiding this comment

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

LGTM

setFees(0, 0);
mintAndDepositIntoStrategy(strategy, _address, _amount);

uint256 toLoose = _amount;
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean toLose? The amount that will be lost -> amount to lose?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Schlagonia Schlagonia merged commit e031090 into 3_0_3 Sep 18, 2024
3 checks passed
@Schlagonia Schlagonia deleted the convert branch September 18, 2024 23:22
Schlagonia added a commit that referenced this pull request Sep 24, 2024
* chore: bump api version

* test: fix invariant test

* feat: max deposit (#99)

* feat: max uint deposit

* fix: invariant test

* fix: rebase

* fix: dont max mint

* fix: uint 256

Co-authored-by: FP <[email protected]>

* fix: uint 256

Co-authored-by: FP <[email protected]>

* test: check balances

---------

Co-authored-by: FP <[email protected]>

* chore: add statemind audit

* chore: match abis (#100)

* feat: update name (#101)

* chore: update domain separator (#102)

* chore: update domain seperator

* feat: dont cache domain

* chore: test runs

* fix: comment

Co-authored-by: spalen0 <[email protected]>

* fix: comment

Co-authored-by: spalen0 <[email protected]>

* fix: permit cast (#104)

* fix: deposit flow (#103)

* fix: conversion

* test: full loss

* feat: add self check to max views

* test: balance check

* fix: spelling

* chore: deployed

---------

Co-authored-by: FP <[email protected]>
Co-authored-by: spalen0 <[email protected]>
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