Conversation
Co-authored-by: Bastian Müller <bastian@turbolent.com>
|
given the huge implications it would have forgetting to unlock a pid, I would suggest adding the following to all the functions that lock post {
self.positionLock[pid] == nil: "Position is not unlocked"
} |
jordanschalm
left a comment
jordanschalm
left a comment
There was a problem hiding this comment.
+1 to Patrick's suggestion about using pre/post-conditions for the position locking:
- functions which expect their caller to have locked a pid should have pre-condition to that effect
- functions which lock a pid should have a post-condition verifying that the pid was unlocked
| /// is pushed to the position's configured `drawDownSink`. | ||
| /// TODO(jord): ~100-line function - consider refactoring. | ||
| access(EPosition) fun depositAndPush( | ||
| access(self) fun _depositEffectsOnly( |
There was a problem hiding this comment.
Can we add documentation for this function?
| self._rebalancePositionNoLock(pid: pid, force: force) | ||
| self._unlockPosition(pid) | ||
| } | ||
| access(self) fun _rebalancePositionNoLock(pid: UInt64, force: Bool) { |
There was a problem hiding this comment.
Could we add documentation for this function?
| [ 0 as UInt64 ], | ||
| PROTOCOL_ACCOUNT | ||
| ) | ||
| Test.expect(updatePositionRes, Test.beSucceeded()) |
There was a problem hiding this comment.
Are there any other expected side effects of the position update we can test here besides that the transaction succeeded? I think the user's position collateral amount (denominated in Flow) should have increased, and the user's vault which backs their top-up source should have decreased.
If this is the only test case we have for the async update function, I'd also suggest adding an issue to expand test coverage (no top-up source, top-up source insufficient, position has queued updates, etc.)
There was a problem hiding this comment.
this test is specifically to test locking mechanism, but I can add more checks here
| // TODO(jord): Sink/source should be valid | ||
| } | ||
| post { | ||
| self.positionLock[self.nextPositionID] == nil: "Position is not unlocked" |
There was a problem hiding this comment.
might be worth noting that this snapshots the self.nextPositionID, and doesn't take the self.nextPositionID = self.nextPositionID + 1 3 lines down, even though this check happens "after" that line is executed
There was a problem hiding this comment.
or, actually, i guess should the before() function be used here?
or maybe result.id?
6 participants
Closes: https://github.com/onflow/flow-consumer-defi-internal/issues/2
Description
For contributor use:
masterbranchFiles changedin the Github PR explorer