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

dex: move value circuit breaker accounting to dex engine boundaries #4025

Closed
zbuc opened this issue Mar 14, 2024 · 1 comment · Fixed by #4086
Closed

dex: move value circuit breaker accounting to dex engine boundaries #4025

zbuc opened this issue Mar 14, 2024 · 1 comment · Fixed by #4086
Assignees
Labels
A-dex Area: Relates to the dex consensus-breaking breaking change to execution of on-chain data _P-high High priority _P-V1 Priority: slated for V1 release state-breaking breaking change to on-chain data
Milestone

Comments

@zbuc
Copy link
Member

zbuc commented Mar 14, 2024

Currently the DEX value circuit breaker is implemented within put_position, however this has two drawbacks:

  1. Misuse resistance -- what if some code modified positions without calling put_position? I audited the codebase for any patterns like this during the initial implementation and couldn't find any, however it could appear in the future. However that seems like a "dark pattern" in general as put_position has various other state machine checks and handles updating indices and liquidity
  2. More importantly, an implementation at the boundaries will be significantly more efficient, as not every position update requires updating and checking the value circuit breaker

Instead, there are only two places that value flows into the dex and two places value flows out:

IN:

  • swap inputs
  • opened liquidity positions

OUT:

  • swap outputs
  • withdrawn positions

If we move the value circuit breaker management to the entry points for those operations, we are guaranteed to check that dex inflows > outflows and get the same assurance with fewer CPU cycles.

@github-project-automation github-project-automation bot moved this to 🗄️ Backlog in Penumbra Mar 14, 2024
@erwanor
Copy link
Member

erwanor commented Mar 14, 2024

Just a note: this is state breaking because we want to split tracking of asset balances into individualized state keys that map asset id to an Amount. Another reason to do this soon is that we want to fix the delta_2 bug asap, and point Zellic to the actual value breaker approach that we will use.

@erwanor erwanor added consensus-breaking breaking change to execution of on-chain data A-dex Area: Relates to the dex state-breaking breaking change to on-chain data labels Mar 15, 2024
@zbuc zbuc added this to the Sprint 2 milestone Mar 18, 2024
@zbuc zbuc moved this from 🗄️ Backlog to Todo in Penumbra Mar 18, 2024
@zbuc zbuc self-assigned this Mar 18, 2024
@hdevalence hdevalence added _P-V1 Priority: slated for V1 release _P-high High priority labels Mar 22, 2024
hdevalence added a commit that referenced this issue Mar 22, 2024
Closes #4025

This commit sketches a new mechanism but does not fix the existing tests; it
needs to be picked up and pushed over the finish line. (We should be testing
this).
zbuc pushed a commit that referenced this issue Mar 22, 2024
Closes #4025

This commit sketches a new mechanism but does not fix the existing tests; it
needs to be picked up and pushed over the finish line. (We should be testing
this).
hdevalence added a commit that referenced this issue Mar 25, 2024
Closes #4025

This commit sketches a new mechanism but does not fix the existing tests; it
needs to be picked up and pushed over the finish line. (We should be testing
this).
hdevalence added a commit that referenced this issue Mar 25, 2024
Closes #4025

This commit sketches a new mechanism but does not fix the existing tests; it
needs to be picked up and pushed over the finish line. (We should be testing
this).
hdevalence added a commit that referenced this issue Mar 25, 2024
Closes #4025

This commit sketches a new mechanism but does not fix the existing
tests; it needs to be picked up and pushed over the finish line. (We
should be testing this).

---------

Co-authored-by: Chris Czub <[email protected]>
@github-project-automation github-project-automation bot moved this from Todo to Done in Penumbra Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dex Area: Relates to the dex consensus-breaking breaking change to execution of on-chain data _P-high High priority _P-V1 Priority: slated for V1 release state-breaking breaking change to on-chain data
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants