This repository has been archived by the owner on Nov 15, 2023. It is now read-only.
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.
Offchain Phragmén BREAKING. #4517
Offchain Phragmén BREAKING. #4517
Changes from 35 commits
46ecd58
a830ec9
8448ee9
5e26a68
65b2df6
85b726c
bf053c2
ca68e24
62e61db
c4a77ac
31b699a
c5d8c51
6d4494e
35a7872
0540646
da66da2
d95010d
3f23367
3cb47fb
a21e90b
df7676b
6f4a2fe
889b18c
50f5399
2bbdec4
c9cfcb8
3633a77
686d263
295cdd7
57b2b4e
6a33fc1
4e951af
5ab3cf0
c88df34
31fd60e
2623d98
c911a87
2ed0a1d
6bef2d9
201c11c
eef09e5
51fc865
aa6186c
596bc77
084a355
817f8aa
d1e290f
02c4e3b
aea7ad2
e2210cc
ca20033
b2c4416
10e97e3
19cf777
54a5a35
d746fd4
2f75ea7
2fa064b
0afbde3
16d8c64
b8efef4
e72d642
9f227f9
9116285
36bbf4b
44c0a6b
c3769fa
f21df59
f9f92c4
4df9ed4
037057c
2b89c0e
4e44501
5c3fc6c
7287c2b
ac6ce78
7be2268
1bd7850
5f9844d
555007f
c16ad73
c8cc72f
ceef0e6
62d4f5e
fd9816a
85f1aa4
9b8af93
58fcf27
7f47696
ef7eab2
5f70434
4d82b4d
58267dc
bee2f5d
3def57b
9590986
6566d81
b01000e
1667f6e
155e9a3
e25c103
81ddd48
87bb4d1
eda82ae
56d1f5e
0c9c20f
f308817
f014e2c
26d9329
ecd042e
63eb8c4
3cd2c55
1e4291c
918e58e
537d6ee
98f9c1b
b553cf5
199d8fb
a424a94
42dbf06
202db20
8372453
db50363
2692605
2cf090c
ffda508
4c56e36
18558af
6f9a2c5
cd3df80
1153084
4c274ab
c75f441
34e3d7b
ee3c4cb
3c5dd95
5a1aa2f
03bb2a9
8dd2564
4514432
ce2ff51
9c0f42c
94efcff
8182947
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
so as discussed, we use the babe keys (same as used for block authoring) for signing and verifying everything here.
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.
Why? Why not separate keys, as for everything?
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.
tbh I think im-online and staking should have no keys of their own and just use whatever the validators are using for their block authoring.
What is the reason to have separate key ? (aside form being similar to other modules)
Code-wise, it is a minor change so no big deal anyway.
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.
We have introduced application keys exactly for making every "application" (Babe, Parachain, etc) use its own specific key.
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.
You also say it was discussed, where was this discussed?
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.
Was somewhere in a group chat. Can't dig it up now. I am fine with both if having a new key type is more conventional
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.
@kianenigma I think
imonline
is using it's own keys (seepallet_im_online::sr25519::AuthorityId
), I guess we should do the same forstaking
. It's a little bit more involved though cause we need to listen toSession
events and rotate them, perhaps this could somehow be abstracted.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.
Yeah,
im-online
is using its own keys.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 declare surrender and will add app keys :D
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.
Wouldn't this make more sense?
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.
It makes more sense given the caller. If we return zero, if the caller is comparing this value to be
le
of some threshold, then it will always think that the condition is met. While by returning max_value we say to the caller infinite blocks are remaining, so that no condition is triggered.But this is tailored for the staking use case. I will change to to returning just some option.
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 had this exact same question. Option does seem better.
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 marked this as resolved by mistake. Will refactor, option is better.
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.
Not done.
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 a bit more about it.
if
checked_sub
fails here, it means a very severe bug in babe is happening. I will add a warning log to it, but returning None seems like a bit ignorant as well.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.
Nonetheless, finally done.