Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Migrate parachain swaps to Coretime #3714
Migrate parachain swaps to Coretime #3714
Changes from 3 commits
36a6368
37962d0
d60b31e
03aaad2
bd32142
4bd69ab
518a072
296521a
e0a1424
ff0d3d7
2f72d26
b2c5ddd
1953bc2
e419bb7
8986843
7f630c8
cc2efb7
a8f17f7
f5f6dce
113bbcf
89ef634
fce11f9
7351ea6
0bbcfd1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
nit: I don't think
TaskId
uniqueness is enforced anywhere inLeases
, so this effectively swaps all leases sharing aTaskId
. This shouldn't really matter as there should only be one task for bothid
andother
.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.
Good point though!
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.
I thought there could be multiple leases and we do want to swap all of them.
If this is the case, we'd better add another
ensure!
for it?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.
Like this -> f5f6dce
The error code I use is not entirely correct but I don't see a point adding a new one just for a temp call.
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.
No Basti has a point. The "correctness" of the swap has already been verified on the relay chain, we should be very lenient on the Coretime chain. In particular, there could be a race and the lease of one para expired by the time the message arrives at the coretime chain (admittedly super unlikely edge case, but still), we should still do the swap, although one of the leases does not exist.
I mean that race in particular is really super niche, if a user updates that close he could miss the deadline himself already.
Nevertheless, I would err on as little ensures as possible: The swap is confirmed, it actually has to be good.
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.
Yes, I came to the same conclusion and removed everything. Even if someone decides to use the extrinsic directly it can be called again and 'undo' the result.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.