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

feat: intro univ3 and prep seeding scripting for ebtc/wbtc pool #18

Merged
merged 12 commits into from
Feb 20, 2024

Conversation

petrovska-petro
Copy link
Collaborator

tackles #8

brownie run scripts/univ3_management pool_creation_and_init_seeding

Copy link
Collaborator

@sajanrajdev sajanrajdev left a comment

Choose a reason for hiding this comment

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

Review

  • The UniV3 helpes and library were integrally imported from the badger-multisig GreatApeSafe. These include helpers to functions to create and manage liquidity positions, swap, collect fees and visualize the state of LPs.
  • The required mainnet addresses, as well as the Vault and Trops addresses, were added to the address book.
  • A draft for the eBTC pool creation and seeding was developed:
    • Pool is created using the 0.05% fee tier as planned
    • Pool is initialized for a 1:1 price
    • Positions are minted for each one of the planned ranges using the percentages predefined by BALCO.
    • Used LIQ as a dummy token while eBTC is deployed to mainnet

@petrovska-petro Implementations LGTM, my only comment is that we should use this chance to add unit tests for at least the operations that we intend to use. It is ok if these are meant to be run against mainnet instead of Sepolia (as opposed to the existing ones).

My concern relates to the issue in which, not too long ago, we almost execute a swap using this library that had a misestimation of the min_out and could have resulted in drain of value. Considering how often we will use this library, I think we need higher certainty into its accuracy.

@sajanrajdev
Copy link
Collaborator

Tackles #11

@petrovska-petro
Copy link
Collaborator Author

great suggestion @sajanrajdev i added tests on the last commits for the actions intended for now

one thing im still researching is something to note on the test_position_creation that the ticks of the position when converting them from the upper and lower ticks does not match the intended. values are a bit off:

price_lower_ticket=0.9891
price_higher_tick=0.995

run: brownie test tests/univ3/* -s

im still investigating if related to tick spacing

@petrovska-petro
Copy link
Collaborator Author

pushed 5241b91 which i think was a mistaken assumptions inherit from the fact that badgerwbtc and ethgtc shared the same fee tier, but this one suggested in the forum is meant to be 0.05% which by result the tick spacing will be different. now it should represent a more precise representation of the nft positions created given the price targets. will be great if you could also provide feedback if you see something im missing out @sajanrajdev

Copy link

@rayeaster rayeaster left a comment

Choose a reason for hiding this comment

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

LGTM to have scripts to setup UniV3 LP with respect to BALCO decision

_,
_,
_,
) = safe.ebtc.cdp_manager.Cdps(cdp_id)

Choose a reason for hiding this comment

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

same comment added here #19 (comment)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

suggested added in 390a7d9

token_0_init_seeding_amount = 2.5e8 if token0 == wbtc.address else 2.5e18
token_1_init_seeding_amount = 2.5e18 if token1 == liq.address else 2.5e8

# single-side lp only ebtc
Copy link

@rayeaster rayeaster Feb 13, 2024

Choose a reason for hiding this comment

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

do we assume token0 is eBTC here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is still sort of a placeholder in light of non ebtc in prod. order of which one will be token0 or token1 will depending on the internally ordering of the each token address itself when deploying the pool

https://github.com/Uniswap/v3-core/blob/main/contracts/UniswapV3Factory.sol#L41

token_1_init_seeding_amount * PCT_40,
)

# single-side only wbtc

Choose a reason for hiding this comment

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

do we assume token1 is wBTC here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

same as #18 (comment)

Copy link
Collaborator

@sajanrajdev sajanrajdev left a comment

Choose a reason for hiding this comment

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

Additional Review

  • Introduces a test case for an pool creation using the expected specs for eBTC/wBTC (with LIQ as dummy token1 for now)
  • Introduces a test case for the treasury's position creation for the 1 - 1.01 range
  • Allows for tick spacing to be changed dynamically for pools of different fee tiers when minting new positions
  • All tests are passing:
    image

LGTM

positions = treasury.uni_v3.nonfungible_position_manager.positions(token_id)

# TODO: rounding to 4 decimal places, values are not matching the expected?
price_lower_ticket = round(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor typo in here. Mean ticker or tick, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good catch, fixed in 9d360d7

Copy link
Collaborator

@sajanrajdev sajanrajdev left a comment

Choose a reason for hiding this comment

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

LGTM

@petrovska-petro petrovska-petro merged commit f1c9662 into main Feb 20, 2024
3 checks passed
@petrovska-petro petrovska-petro deleted the feat/univ3-helpers branch February 20, 2024 05:41
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