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

Feat: ensure correct process termination in ingress/egress tracker #4101

Merged
merged 7 commits into from
Oct 16, 2023

Conversation

msgmaxim
Copy link
Contributor

@msgmaxim msgmaxim commented Oct 11, 2023

Pull Request

Part of: PRO-868

(addresses a review comment from #4081)

Checklist

Please conduct a thorough self-review before opening the PR.

  • I am confident that the code works.
  • I have updated documentation where appropriate.

Summary

The PR is designed to be ready commit-by-commit. Note that first commit just moves code into a different file without changes (I separated it to make reviewing easier).

The main change is to use task_scope to spawn all long-running tasks (I did not bother with the task inside register_subscription since a panic there doesn't necessarily need to terminate the entire process, and it seemed non-trivial to get lifetimes work there).

@codecov
Copy link

codecov bot commented Oct 11, 2023

Codecov Report

Merging #4101 (4a431e9) into main (eb669d2) will increase coverage by 0%.
Report is 15 commits behind head on main.
The diff coverage is n/a.

@@          Coverage Diff           @@
##            main   #4101    +/-   ##
======================================
  Coverage     71%     72%            
======================================
  Files        377     378     +1     
  Lines      59965   60637   +672     
  Branches   59965   60637   +672     
======================================
+ Hits       42716   43492   +776     
+ Misses     15008   14880   -128     
- Partials    2241    2265    +24     

see 37 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@msgmaxim msgmaxim enabled auto-merge (squash) October 16, 2023 01:45
@msgmaxim msgmaxim merged commit 5c70808 into main Oct 16, 2023
44 checks passed
@msgmaxim msgmaxim deleted the feat/ingress-egress-task-scope branch October 16, 2023 02:18
@linear
Copy link

linear bot commented Oct 16, 2023

PRO-868 Ingress Egress Tracker

This will/should be broken down into smaller tickets after we've discussed it.

Problem:

The swapping app currently has to wait for the CFE witnessing and respective confirmation on the SC, both for ingress, when the user is depositing funds, and for egress, when the user is receiving funds. This duration can be quite long, because of the added safety margins to protect against reorgs.

Solution:

While the reorg safety is important for the protocol it's not important for the product. We can use the same code that we use in the CFE witnessing to avoid duplicating the work, but instead redirect the results of that.

Requirements:

  • The Calls that we would normally send to the SC as an extrinsic, can instead be sent down a WS stream for consumption by product team (it doesn't have to be WS according to product team, but I think WS will be simplest for us to do)
  • Reusing the same witnessing components as the engine currently does where possible.
  • Single binary for the trackers of all the chains - this includes the btc deposit tracker, so would unify / use that deposit tracker binary alongside the code for this
  • Configurable, so it can be used for different networks
    • SC WS endpoint
    • A single, (no need for backup) WS and HTTP endpoints for each chain

Useful info:

  • feat: simple pre-witnessing #4056 / PRO-852
    • Example of how the egress components would be pulled out, as well as the usage of the ProcessCall, and how it can be used to pass in closures to send different data

Out of scope:

  • State, there's no need to hold state in the tracker, either in memory, or in a database. We can be optimistic. If the tracker goes down that's ok, the UI can fallback on the SC BroadcastSuccess event which it already uses, it'll just be a bit slower from the user's perspective for now, but for now that's ok.

If you have questions for the product team and what they need, you can reach out to nat

syan095 added a commit that referenced this pull request Oct 17, 2023
* origin/main:
  chore: get gas parameters from statechain event (#4125)
  Feat: ingress-egress tracking for DOT (#4121)
  add support for hex encoded amounts on limit order and range order methods in LP API (#4120)
  feat: bouncer command for submitting runtime upgrades (#4122)
  Feat: ensure correct process termination in ingress/egress tracker (#4101)
  feat(custom-rpc): add flip balance to account info (#4119)
  chore: storage migration delete NextCompatibilityVersion (#4115)
  chore: delete unneeded function (#4116)

# Conflicts:
#	state-chain/runtime/src/lib.rs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants