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

LP management improvements/fixes #18

Merged
merged 12 commits into from
Jun 1, 2023
Merged

LP management improvements/fixes #18

merged 12 commits into from
Jun 1, 2023

Conversation

EdNoepel
Copy link
Contributor

Changes

  • Added pool to MemorializePosition, lining it up with RedeemPosition.
  • Move ERC20 factory address to lookup table to improve maintainability.
  • Don't create Lend entities in places where we expect them to already exist.
  • Fixed two different bugs preventing 0 LP Lends from being removed.

@EdNoepel EdNoepel changed the title 3008 LP management improvements/fixes May 31, 2023
@EdNoepel EdNoepel requested a review from MikeHathaway May 31, 2023 19:54
@@ -1223,13 +1227,18 @@ export function handleTransferLP(event: TransferLPEvent): void {
const oldLendId = getLendId(bucketId, entity.owner)
const newLendId = getLendId(bucketId, entity.newOwner)

// If PositionManager generated this event, it means either:
Copy link
Contributor

Choose a reason for hiding this comment

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

Would making Lend's persistent help with this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this branch, I originally tried to preserve the LP amount in the lender's Lend upon transfer to PositionManager, and tag the lend with a tokenId. That breaks as soon as someone calls MoveLiquidity, because we cannot determine the lender whose liquidity was moved. Also, a single lender can mint multiple position NFTs, yet they only get one Lend for a bucket.

@@ -27,13 +24,15 @@ export const MAX_PRICE_BI = BigInt.fromString('1004968987606512354182109771')
export const MIN_BUCKET_INDEX = -3232;
export const MAX_BUCKET_INDEX = 4156;

// poolInfoUtils contract address per network
// Pool addresses per network
// TODO: rename these something more palatable, like "AddressTable"
Copy link
Contributor

Choose a reason for hiding this comment

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

AddressTable sounds good. We probably want to also move these into a separate file at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to figure out the best practice for handling them in subgraph.yaml. Would be nice if we could define all the addresses once in the environment or some other central location.

Copy link
Contributor

@MikeHathaway MikeHathaway May 31, 2023

Choose a reason for hiding this comment

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

Yeah, I spent a while trying to do something more elegant than the lookup tables, but that was the best I could find at the time. It seems to have issues with reading that network config directly

Copy link
Contributor

@MikeHathaway MikeHathaway left a comment

Choose a reason for hiding this comment

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

LGTM. Few minor comments

@EdNoepel EdNoepel merged commit 85f3950 into develop Jun 1, 2023
@EdNoepel EdNoepel deleted the 3008 branch June 1, 2023 12:14
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.

2 participants