-
Notifications
You must be signed in to change notification settings - Fork 324
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: fold Position
s into the price indexes
#4564
Conversation
There's an interesting idea about using the pb type for asset id to short-circuit an expensive field element serialization. But if we do it, that will be in a different PR. |
OK, I believe we're unblocked to rebase this one now. Sorry we missed an opportunity to coordinate more closely on starting the Testnet78 migration. @erwanor could you handle the rebase since you know the logic best? Then I'll circle back to test the migration end to end as part of final review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has conflicts due to Testnet78 migration added recently, oops. Requesting rebase resolution, then will test as part of review.
ea38bf1
to
67f8546
Compare
67f8546
to
a49fc48
Compare
I have tested this manually, and rebased onto main. |
Tested this by setting up a devnet on
Then, after voting for a halt and applying the migration, the position is still visible, and I can still swap into it, seeing the balance updated:
That looks right to me! One more migration to write toward 78. |
Describe your changes
This PR:
Position
records into the DEX price indexesId
s)Checklist before requesting a review
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: