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

Limit Orders #181

Merged
merged 45 commits into from
Dec 16, 2024
Merged

Limit Orders #181

merged 45 commits into from
Dec 16, 2024

Conversation

JasonMHasperhoven
Copy link
Contributor

No description provided.

@cronokirby cronokirby marked this pull request as ready for review December 10, 2024 02:23
@hdevalence
Copy link
Member

Screenshot 2024-12-09 at 6 43 41 PM

Running locally I get summary issues -- I think this was already fixed in main, do we think it would disappear on rebase?

@cronokirby
Copy link
Contributor

Screenshot 2024-12-09 at 6 43 41 PM

Running locally I get summary issues -- I think this was already fixed in main, do we think it would disappear on rebase?

I got this error when I forgot to set the environment variable to point at a node

price: number,
pExponent: number,
qExponent: number,
): { p: Amount; q: Amount } => {
Copy link
Member

Choose a reason for hiding this comment

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

Let's prefer using $p_1$ and $p_2$, we realized $p,q$ was messy too late in the implementation lifecycle, but we can still correct course here

),
qExponent,
).toAmount();

Copy link
Member

Choose a reason for hiding this comment

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

Do we need scale at all?

// Pseudo code
const p_1 = round(quoteAssetExponent * price);
const p_2 = baseAssetExponent;

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the exponent shift.

src/shared/math/position.ts Show resolved Hide resolved

// const r1 = pnum(plan.baseReserves, plan.baseAsset.exponent).toAmount();
// const r2 = pnum(plan.quoteReserves, plan.quoteAsset.exponent).toAmount();
const r1 = pnum(plan.baseReserves).toAmount();
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to make sure that base is asset_1 and quote is asset_2 and flip everything if it's not.

Ideally, there's a Position constructor that does this on your behalf so that you don't have to worry about it.

@hdevalence
Copy link
Member

Some things I tested but couldn't get working:

  1. Go to https://localhost/trade/USDY/USDC, try to create a "buy" limit order to buy 1 USDY @ 1:1 using 1 USDC. this fails with a negative exponent error

  2. Same page, opposite direction, try to create a "sell" limit order to sell 1 USDC @ 1:1. this fails first with a missing source field (was undefined, pushed a commit that works around though i'm not sure it's the right approach), then I got an error that I didn't have any USDY. Reserves don't look correct to me, but I'm not sure why?

USDY/USDC limit orders are a good test case because they exercise different denoms and also unlock a market-based solution to the USDC transfer issue.

Screenshot 2024-12-10 at 7 22 24 PM Screenshot 2024-12-10 at 7 22 46 PM Screenshot 2024-12-10 at 7 23 07 PM

@cronokirby
Copy link
Contributor

I refactored the stores to all have a much more consistent set of transitions and a reduced set of truth, and everything now seems to be working correctly.

I clobbered some work in the process, which is backed up in https://github.com/penumbra-zone/dex-explorer/tree/limitorders-archive just in case.

There were a handful of bugs not even reported in slack across the various forms, but now it should all work.

I tested it buy working on the testnet, using TestUSD and UM, which have 1e18 and 1e6 as the exponent

Range Liquidity:
image
image
The amount of um and test usd being consumed is right on the website, but also in the extension. Also, the base unit price is 1.7e12 in the first screenshot, which is accurate, given the exponent difference.

Limit Order
Buy
image
image
Sell
image
image

Some testing on mainnet would be appreciated

@cronokirby cronokirby marked this pull request as ready for review December 12, 2024 08:32
This should handle cases where they're out of range by losing only the necessary precision, and it should handle cases where the price gets flattened to 0
@JasonMHasperhoven
Copy link
Contributor Author

We have arrived:
https://github.com/user-attachments/assets/f5a85d5f-202d-48af-92a6-e3c196c7ca82

@JasonMHasperhoven JasonMHasperhoven requested a review from a team December 16, 2024 14:33
@JasonMHasperhoven JasonMHasperhoven changed the title [WIP] Limit Orders Limit Orders Dec 16, 2024
Copy link
Contributor

@grod220 grod220 left a comment

Choose a reason for hiding this comment

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

Great job (everyone)! Given the number of devs that have contributed to this PR, think we should feel confident to ship and iterate as needed in follow ups 👍

@JasonMHasperhoven JasonMHasperhoven merged commit 699f741 into main Dec 16, 2024
3 checks passed
@JasonMHasperhoven JasonMHasperhoven deleted the limitorders branch December 16, 2024 16:23
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.

5 participants