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: add initiated_at block number for egresses #4046

Merged
merged 3 commits into from
Sep 27, 2023

Conversation

kylezs
Copy link
Contributor

@kylezs kylezs commented Sep 25, 2023

Pull Request

First PR of: PRO-689
Second PR will include the CFE side. Figured would split this since it's possible.

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

  • Adds an initiated_at to the on_signature_ready_callback, such that we set it to the earliest possible block number it could be in.
  • In the next PR, the CFE will use this to "hold up" external chain blocks it is not yet ready to process, avoiding the race condition.

@codecov
Copy link

codecov bot commented Sep 25, 2023

Codecov Report

Merging #4046 (872a519) into main (61dbd66) will decrease coverage by 0%.
Report is 5 commits behind head on main.
The diff coverage is 38%.

@@          Coverage Diff           @@
##            main   #4046    +/-   ##
======================================
- Coverage     72%     72%    -0%     
======================================
  Files        369     370     +1     
  Lines      59126   59306   +180     
  Branches   59126   59306   +180     
======================================
+ Hits       42651   42674    +23     
- Misses     14357   14508   +151     
- Partials    2118    2124     +6     
Files Coverage Δ
state-chain/pallets/cf-broadcast/src/mock.rs 93% <100%> (+<1%) ⬆️
state-chain/pallets/cf-broadcast/src/tests.rs 97% <100%> (+<1%) ⬆️
state-chain/runtime/src/lib.rs 39% <ø> (ø)
state-chain/pallets/cf-broadcast/src/lib.rs 82% <80%> (-<1%) ⬇️
...ate-chain/pallets/cf-broadcast/src/benchmarking.rs 0% <0%> (ø)
...ts/cf-broadcast/src/migrations/add_initiated_at.rs 0% <0%> (ø)

... and 30 files with indirect coverage changes

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

@kylezs kylezs force-pushed the fix/egress-id-race-condition branch from 6be3117 to e7bf8cf Compare September 25, 2023 14:40
@kylezs kylezs requested a review from dandanlen September 25, 2023 14:42
@kylezs kylezs marked this pull request as ready for review September 25, 2023 14:42
@kylezs kylezs force-pushed the fix/egress-id-race-condition branch from e7bf8cf to a39d37e Compare September 26, 2023 06:18
Copy link
Collaborator

@dandanlen dandanlen left a comment

Choose a reason for hiding this comment

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

LGTM. Just a note that the runtime upgrade isn't being run, it needs to be added in the hooks.

@@ -179,6 +182,9 @@ pub mod pallet {

type BroadcastReadyProvider: OnBroadcastReady<Self::TargetChain, ApiCall = Self::ApiCall>;

/// Get the latest block height of the target chain via Chain Tracking.
type ChainTracking: GetBlockHeight<Self::TargetChain>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Side note: we should consider adding ChainTracking to the Chainflip trait - it's getting its tentacles in everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This made me think that maybe it makes sense to merge the broadcast and ingress-egress pallet actually... after all, the broadcast is egressing transactions... something we can think about anyway.

@kylezs kylezs enabled auto-merge (squash) September 27, 2023 08:13
@kylezs kylezs merged commit 3c2afa8 into main Sep 27, 2023
44 checks passed
@kylezs kylezs deleted the fix/egress-id-race-condition branch September 27, 2023 08:42
dandanlen pushed a commit that referenced this pull request Sep 28, 2023
* feat: add initiated_at block number for egresses

* chore: add migration for chain height addition

* chore: add migration hooks
dandanlen pushed a commit that referenced this pull request Sep 29, 2023
* feat: add initiated_at block number for egresses

* chore: add migration for chain height addition

* chore: add migration hooks
dandanlen pushed a commit that referenced this pull request Oct 9, 2023
* feat: add initiated_at block number for egresses

* chore: add migration for chain height addition

* chore: add migration hooks
@linear linear bot mentioned this pull request Nov 13, 2023
2 tasks
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