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

update Position schema; add PositionLend entity #62

Merged
merged 20 commits into from
Aug 24, 2023

Conversation

MikeHathaway
Copy link
Contributor

@MikeHathaway MikeHathaway commented Aug 23, 2023

  • Add PositionLend entity and associate with Position and Bucket in many-to-one relationship.
  • Associate Position and Account entities
  • Add tokenURI attribute to Position entity
  • Record lpb and lpbValueInQuote for each PositionLend associated with a Position by calling getPositionInfo
  • Refactor MoveLiquidity and remove Lend update logic as this is already handled in the associated MoveQuoteToken event
  • Expand MoveLiquidity test

@MikeHathaway MikeHathaway requested a review from EdNoepel August 23, 2023 22:20
@MikeHathaway MikeHathaway marked this pull request as ready for review August 23, 2023 22:20
Copy link
Contributor

@EdNoepel EdNoepel left a comment

Choose a reason for hiding this comment

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

Two concerns noted regarding mutating copies of collections.

}
const positionLend = loadOrCreatePositionLend(redeem.tokenId, bucketId, index)
positionLend.lpb = ZERO_BD
positionLend.lpbValueInQuote = ZERO_BD
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems unnecessary to mutate anything besides lpb, because the entity will be removed from the store.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

for (let i = 0; i < bucket.positionLends.length; i++) {
const positionLendId = bucket.positionLends[i]
const positionLend = PositionLend.load(positionLendId)!
positionLend.depositTime = bucketBankruptcy.blockTimestamp.plus(ONE_BI)
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious why we would update anything aside from positionLend.lpb here. By setting lpb to 0, saveOrRemovePositionLend should always call store.remove, so these mutations would never be persisted. Regardless, advancing depositTime doesn't seem appropriate.

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 will remove those updates. The depositTime advancing matches the contract logic

// remove position from old account
const oldOwnerAccount = loadOrCreateAccount(transfer.from)
const index = oldOwnerAccount.positions.indexOf(bigIntToBytes(transfer.tokenId))
if (index != -1) oldOwnerAccount.positions.splice(index, 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

That's not going to work. When we call entity.collection, it returns a copy of the collection. Mutations on this collection will not persist when save is called on line 296.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, and also updated in handleBurn.

const bucket = Bucket.load(positionLend.bucket)!
const existingBucketIndex = bucket.positionLends.indexOf(positionLend.id)
if (existingBucketIndex != -1) {
bucket.positionLends.splice(existingBucketIndex, 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

This and line 104 mutate a copy of the collection, and will not effect the entity, even when the caller calls .save.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated accordingly

@MikeHathaway MikeHathaway requested a review from EdNoepel August 24, 2023 02:03
Copy link
Contributor

@EdNoepel EdNoepel left a comment

Choose a reason for hiding this comment

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

LGTM; thank you!

@MikeHathaway MikeHathaway merged commit aa0548f into develop Aug 24, 2023
@MikeHathaway MikeHathaway deleted the position-updates branch August 24, 2023 14:43
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