Remove absolute position update #512
Open
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
kbrizzle
reviewed
Dec 13, 2024
Merged
…30-remove-absolute-position-update
|
This one looks like it's on the right track now 👍 |
…30-remove-absolute-position-update
EdNoepel
reviewed
Dec 23, 2024
| expect((await market.pendingOrders(user.address, 2)).protection).to.eq(1) | ||
| }) | ||
|
|
||
| it('liquidates a user with close', async () => { |
There was a problem hiding this comment.
Note to self: We can no longer liquidate with update, only close, explaining why two tests are no longer needed here.
| }, | ||
| ] | ||
| } | ||
|
|
| @@ -761,16 +760,16 @@ export function RunManagerTests(name: string, getFixture: (overrides?: CallOverr | |||
| expect(canExecuteBefore).to.be.false | |||
|
|
|||
| // time passes, other users interact with market | |||
| let positionA = (await market.positions(userA.address)).maker | |||
| let positionC = (await market.positions(userC.address)).short | |||
| let positionADelta | |||
There was a problem hiding this comment.
nit: I would have named this positionDeltaA to prevent jumbling uppercase letters together.
| /// @param timestamp The current timestamp | ||
| /// @param position The current position | ||
| /// @param makerAmount The magnitude and direction of maker position | ||
| /// @param takerAmount The magnitude and direction of taker position | ||
| /// @param collateral The change in the collateral | ||
| /// @param protect Whether the order is protected |
There was a problem hiding this comment.
Suggested change
| /// @param protect Whether the order is protected | |
| /// @param protect True when liquidating the position |
| @@ -80,12 +80,13 @@ library OrderLib { | |||
| (UFixed6Lib.ZERO, UFixed6Lib.ZERO, UFixed6Lib.ZERO, UFixed6Lib.ZERO, UFixed6Lib.ZERO, UFixed6Lib.ZERO); | |||
| } | |||
|
|
|||
| /// @notice Creates a new order from the an intent order request | |||
| /// @notice Creates a new order from the an intent and delta order request | |||
There was a problem hiding this comment.
from the an intent and delta doesn't sound right. If this from method is really used for both use cases, it could read from an intent or delta.
kbrizzle
reviewed
Dec 27, 2024
| newOrder.makerReferral = makerAmount.abs().mul(referralFee); | ||
| newOrder.takerReferral = takerAmount.abs().mul(referralFee); | ||
| newOrder.protection = protect ? 1 : 0; |
There was a problem hiding this comment.
do we need to change the order here? might want to leave in the original slot just to reduce the changeset.
There was a problem hiding this comment.
I think it happened while merging, reverting to old order.
| /// @param collateral The collateral delta of the order (positive for deposit, negative for withdrawal) | ||
| /// @param referrer The referrer of the order | ||
| function update( | ||
| address account, | ||
| Fixed6 amount, | ||
| Fixed6 takerAmount, |
There was a problem hiding this comment.
what if we went the other direction for these and named them taker and maker like you did in the Trigger Order code for both overloads? might be a little more succint.
There was a problem hiding this comment.
In the trigger orders i have used makerDelta and takerDelta. I think taker and maker are used in multiInvoker here. I agree that it will be more succinct but I like takerAmount more than taker as it helps with readability and think that we should rename to takerAmount in multiInvoker as well.
| .muldiv(marketContext.registration.leverage, marketContext.latestPrice.abs()) | ||
| .max(marketContext.minPosition) | ||
| .min(marketContext.maxPosition); | ||
| target.position = _getTargetPosition(marketContext, marketAssets); |
There was a problem hiding this comment.
Instead of pulling this out into it's own function what if we did:
- change
target.positiontoUFixed6 newMaker. - add new line
target.position = Fixed6Lib.from(newMaker).sub(Fixed6Lib.from(marketContext.currentAccountPosition.maker));
| /// @dev The new position | ||
| UFixed6 position; | ||
| /// @dev The amount of change in position | ||
| Fixed6 position; |
There was a problem hiding this comment.
maybe we should change the name of this field to taker? not that's not representing a absolute position anymore?
There was a problem hiding this comment.
I think you mean change this to maker? Maybe we can change this to makerDelta or positionDelta?
[Periphery] Unit Test Coverage ReportCoverage after merging prateek/pe-1630-remove-absolute-position-update into v2.4 will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
[Core] Integration Test Coverage ReportCoverage after merging prateek/pe-1630-remove-absolute-position-update into v2.4 will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Code Size Diff:View Report |
Gas Report Diff:View Report |
[Periphery] Integration Test Coverage ReportCoverage after merging prateek/pe-1630-remove-absolute-position-update into v2.4 will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
[Periphery] Combined Test Coverage ReportCoverage after merging prateek/pe-1630-remove-absolute-position-update into v2.4 will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
[Core] Unit Test Coverage ReportCoverage after merging prateek/pe-1630-remove-absolute-position-update into v2.4 will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
[Core] Combined Test Coverage ReportCoverage after merging prateek/pe-1630-remove-absolute-position-update into v2.4 will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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.
Issues addressed: