ToB improvements #736
Merged
ToB improvements #736
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…’s intended behavior
grandizzy
requested review from
EdNoepel,
ith-harvey,
mattcushman,
prateek105 and
MikeHathaway
grandizzy
changed the title
MikeHathaway
approved these changes
Apr 10, 2023
| @@ -21,7 +21,6 @@ interface IPositionManagerOwnerActions { | |||
| * @dev The array of buckets is expected to be constructed off chain by scanning events for that lender. | |||
| * @dev The NFT must have already been created, and the number of buckets to be memorialized at a time determined by function caller. | |||
| * @dev An additional call is made to the pool to transfer the LPs from their previous owner, to the Position Manager. | |||
| * @dev Pool.setPositionOwner() must be called prior to calling this method. | |||
There was a problem hiding this comment.
Should there be some sort of comment pointing to the need to approve the lp transfers?
There was a problem hiding this comment.
good point, can you suggest a nice wording for this? And should be applied only when memorializing position, and not for reedemPositions if I am not wrong?
There was a problem hiding this comment.
would this work d58fe44 ?
There was a problem hiding this comment.
I actually got this right with 0a50f8e :)
There was a problem hiding this comment.
Just saw the request - yeah that looks great!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ensure that comments in code always reflect the code’s intended behavior.
Deposits.sol#L195says the following code isCase 1 abovebut it’s actually case 2.Pool.sol#L702says External Functions but is more precisely External View Functions.IPositionManagerOwnerActions.sol#L24andL54, the references tosetPositionOwnerare incorrect.Contracts/README.MD#L8, remove references tocryptopunksandcrypto kitties.BorrowerActions#L292, the comment should read: revert if no amount to pull or repay.Auctions.sol#L1269, should be removed, astotalBondEscrowedis updated in the Pool’swithdrawBondsfunction.Constant variable WAD is declared but not used in Maths.sol.
Add tests for
Maths.maxIntandMaths.minIntIn
LenderActions.sol#L185-191should useBuckets.addLenderLPs.