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

Fix routable asset ordering #4193

Merged
merged 10 commits into from
Apr 29, 2024
Merged

Fix routable asset ordering #4193

merged 10 commits into from
Apr 29, 2024

Conversation

zbuc
Copy link
Member

@zbuc zbuc commented Apr 11, 2024

Describe your changes

Fixes the ordering of routable assets by liquidity as well as adds tests.

Issue ticket number and link

Closes #4189

Checklist before requesting a review

  • If this code contains consensus-breaking changes, I have added the "consensus-breaking" label.

@zbuc zbuc added the consensus-breaking breaking change to execution of on-chain data label Apr 11, 2024
cratelyn added a commit that referenced this pull request Apr 12, 2024
in #4193, @zbuc is working on some changes to the dex component.
in the `penumbra-app` crate, we have a helpful utility to define a
tracing subscriber that correctly interacts with `libtest`'s output
capturing, and can be configured via `RUST_LOG` when running tests.

this is a tremendously helpful aid in debugging issues during feature
development, and is not specific to the `penumbra-app` crate.

this commit hoists that test utility into a standalone library in
`crates/test/`, so that other components like the dex can also create a
guard to capture traces in tests.

now we can share this, by doing this:

```rust
 #[tokio::test]
 async fn example_test_case() -> anyhow::Result<()> {
     // Install a test logger...
     let guard = set_tracing_subscriber();

     // Test logic here...

     drop(guard);
     Ok(())
 }
```
cratelyn added a commit that referenced this pull request Apr 12, 2024
in #4193, @zbuc is working on some changes to the dex component. in the
`penumbra-app` crate, we have a helpful utility to define a tracing
subscriber that correctly interacts with `libtest`'s output capturing,
and can be configured via `RUST_LOG` when running tests.

this is a tremendously helpful aid in debugging issues during feature
development, and is not specific to the `penumbra-app` crate.

this commit hoists that test utility into a standalone library in
`crates/test/`, so that other components like the dex can also create a
guard to capture traces in tests.

now we can share this, by doing this:

```rust
 #[tokio::test]
 async fn example_test_case() -> anyhow::Result<()> {
     // Install a test logger...
     let guard = set_tracing_subscriber();

     // Test logic here...

     drop(guard);
     Ok(())
 }
```

#### checklist before requesting a review

- [x] If this code contains consensus-breaking changes, I have added the
"consensus-breaking" label. Otherwise, I declare my belief that there
are not consensus-breaking changes, for the following reason:

  > only changes test code
@zbuc zbuc force-pushed the 4189_be_liquidity_index branch from df14206 to 5b54860 Compare April 12, 2024 20:59
@zbuc zbuc requested a review from erwanor April 12, 2024 21:08
@zbuc
Copy link
Member Author

zbuc commented Apr 12, 2024

@erwanor we should be good to clean this up and work on a migration in a pairing session when you've got time

@erwanor erwanor added the state-breaking breaking change to on-chain data label Apr 13, 2024
@cratelyn cratelyn added the A-dex Area: Relates to the dex label Apr 16, 2024
@cratelyn cratelyn added this to the Sprint 4 milestone Apr 16, 2024
@zbuc zbuc force-pushed the 4189_be_liquidity_index branch from 5b54860 to 4f954f2 Compare April 17, 2024 19:20
Copy link
Member

@erwanor erwanor left a comment

Choose a reason for hiding this comment

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

LGTM, we could add a couple lines to the AssetByLiquidityIndex trait about the intuition around manipulation-resistance and why we expect it to be directionally correct even if it doesn't factor the price

@zbuc zbuc marked this pull request as ready for review April 17, 2024 20:20
@erwanor erwanor self-requested a review April 18, 2024 14:06
@cratelyn cratelyn modified the milestones: Sprint 4, Sprint 5 Apr 22, 2024
@zbuc zbuc force-pushed the 4189_be_liquidity_index branch from 93782db to 8af60e6 Compare April 26, 2024 17:36
@cratelyn cratelyn added C-bug Category: a bug _P-V1 Priority: slated for V1 release labels Apr 26, 2024
@zbuc zbuc merged commit 9a7b43d into main Apr 29, 2024
13 checks passed
@zbuc zbuc deleted the 4189_be_liquidity_index branch April 29, 2024 22:42
@erwanor erwanor added the migration A pull request that contains a migration label May 7, 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 C-bug Category: a bug consensus-breaking breaking change to execution of on-chain data migration A pull request that contains a migration _P-V1 Priority: slated for V1 release state-breaking breaking change to on-chain data
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

dex: use big endian of complement in the base liquidity index
3 participants