-
Notifications
You must be signed in to change notification settings - Fork 15
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
Feat: Core DCA #5106
Feat: Core DCA #5106
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5106 +/- ##
======================================
- Coverage 72% 71% -0%
======================================
Files 453 454 +1
Lines 81473 81768 +295
Branches 81473 81768 +295
======================================
+ Hits 58268 58451 +183
- Misses 20111 20205 +94
- Partials 3094 3112 +18 ☔ View full report in Codecov by Sentry. |
None, | ||
None, |
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.
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.
As mentioned in the previous PR, I think it's worth distinguishing more strongly between id types just for peace of mind. I've created a Linear issue PRO-1548.
state-chain/pallets/cf-ingress-egress/src/migrations/add_refund_params.rs
Outdated
Show resolved
Hide resolved
(Will continue reviewing tomorrow) |
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.
Actually a few more comments here. More to follow tomorrow.
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.
Some more 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.
Thanks, looking good 🚀 . I'll take a closer look at tests on Monday.
Migrations / storage versions might be impacted, this PR was required because otherwise CI on main doesn't pass: #5142 |
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.
Test cases look fine to me. Although I feel we can do better to reduce duplication in the tests and make them more readable - they are quite verbose and repetitive (this applies to pre-existing tests too, not just the newly added ones). For example, the use of 'global' constants for test amounts etc makes things a little hard to follow.
I think it's time we added some simple extension traits with common setup/update/check methods that are used across all the swapping tests. I'll raise an issue.
Apart from that I believe the only things that needs changing is what was discussed this morning re. expiry being implicit from the FoK retry delay and number of dca chunks - I think this implies simply updating the refund params for every chunk.
Pull Request
Closes: PRO-1531, PRO-1539
Checklist
Please conduct a thorough self-review before opening the PR.
Summary
I'm hoping to merge this core functionality early so that @j4m1ef0rd could start working on this in parallel (we can/might also work on this branch ofc, but that's not ideal). This means some important features might come in separate PRs (see below).
dca_parameters
to therequest_swap_deposit_address_with_affiliates
extrinsic and the the broker rpc method; the parameters are saved on the channel's actions (similarly to refund params). New migration adds None to any existing channels.RequestState
. Now when a swap completes, we check DCA state to see what should happen next: if the swap/chunk is successful, we either schedule the next chunk (recording output) or egress if this was the final chunk; on failure, we refund any unexecuted amount, but still egress output from any previous chunks. No migration is needed sinceRequestState
s don't exist in 1.5.Out of scope:
Update: since this is early in review, I pushed another commit that now handles CCM too.