-
Notifications
You must be signed in to change notification settings - Fork 4
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
Feature/settlement rewrite #202
Conversation
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.
From a preliminary review the code looks good! It is difficult to tell if the logic is 100% correct simply from the code, so I await the e2e test to do a more thorough review.
Besides tests and benchmarks, what features are missing compared to main
?
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.
General direction seems very good.
A lot of changes were done, so I focused more on the structure than the internal logic.
A last review should be done after the PR is marked as ready.
When it's marked as ready I would like to see an in-detail description about the most important changes so we are all on the same page. Specially about the migrations. What was happening before, and how it's happening now.
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.
Really really good changes Just, well done!
It feels like we are finally transitioning from a spaghetti to a production-ready codebase.
My main request is to fix the benchmarks cause they aren't working. Try the command just dry-run-benchmarks
to see what I mean.
The rest you'll see on the specific comments :)
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.
Missing fixes and tests that will be addressed in future PRs, but lets merge
What?
This Pr removes the old settlement code and replaces it with new functions. This Pr makes the following big changes to the code:
Settlement:
UserMigrations
Migrations:
do_migrate_one_participant
takes all the users Migrations and sends them over to the destination chain. The QueryId associated with the Xcm is stored in the ActiveMigrationQueue that stored QueryId -> (project_id, participant) and is used to get the correct information associated with the Query id from the UserMigrations storage.Rest:
Why?
To improve code maintainability and quality
How?
Testing?
Screenshots (optional)
Anything Else?