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
feat!: deprecate soft opt-out #1964
feat!: deprecate soft opt-out #1964
Changes from 12 commits
69fff54
fdcca88
621e112
3cb17be
dee262c
e22d3a3
9d2f0b6
e37808a
a423de0
1aed85f
a4ef241
59e46f9
82f5763
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.
ConsumerParams
is used to send data to the consumer chain "over the wire" - it is used in the consumer genesis.Setting it to deprecated allows the field to exist but it will always be empty, thus maintaining backward compatibility.
We will not need to write a special parser to go from v4.2.0
ConsumerParams
to some future versionConsumerParams
.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.
Agreed. This is why we did not use
reserved
.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.
Deprecation note is clear but redundant
The comments on lines 609-611 clearly indicate the deprecation of the Soft Opt-Out feature. However, considering the feature has been deprecated and the parameter is set to "0" to maintain compatibility, these comments might become redundant in the future as developers might overlook them, leading to confusion. It might be beneficial to either remove these comments or update them to reflect why this deprecated feature's parameter still needs to be set.
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 do you need to set it? You can delete this line.
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.
The default value is
""
which is a valid zero string.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 cannot delete this line. If I do, I get the following error:
because the consumer chain never starts. Although we do not utilize
soft_opt_threshold
, the parameter has still to be set. This is why this line existed in all other E2E tests as well. In this PR, I just moved this line to a central place so we do not have to repeat it every time we create a new consumer chain.Yes, we could potentially set simply to
""
here but setting to0
makes it clear that the soft opt-out feature is disabled.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.
Redundant deprecation note
Similar to the previous comment, the redundancy of the deprecation note here could lead to future confusion. Since the parameter is set to "0" and the feature is deprecated, maintaining these comments might not be necessary unless there is a specific reason to keep setting this parameter in the genesis file which should be documented.
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.
Same as above
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.
See my response.
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.
Just a note for posterity: Increased this due to some flakiness in the test involving double signing.
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.
Refactor the use of constants for repeated string literals.
The string
"auto"
is used multiple times across different functions. It is beneficial to define it as a constant at the beginning of the file to ensure consistency and ease of maintenance.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.
Removed this function because it did not add much because we could use
stepsRedelegate
in its place.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 has the ratio changed so much?
Are you trying to check that the soft opt-out no longer works?
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.
TL;DR because we removed opt-out related steps that were reducing the
ValPower
ofalice
.Specifically, we removed the
stepsRedelegateForOptOut
andstepsDowntimeWithOptOut
. ThestepsRedelegateForOptOut
was the one that reduced theValPower
ofalice
to60
so it could test the opt out functionality.Before, in the
happyPathSteps
we had:but we removed
stepsRedelegateForOptOut
andstepsDowntimeWithOptOut
and hence now whenstepsRedelegate
is called, theValPower
ofalice
is not60
anymore but510
. Redelegating400
toalice
makesalice
have aValPower
910
. This meant that allsteps*
functions called afterstepsRedelegate
had to be modified as well to contain the correctValPower
s.No.
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.
Thank you for elaborating. It was not clear to me after going over it a couple times.
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.
This test removes the threshold while the ones above have it stil
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.
Thresholds are removed from the individual
StarConsumerChainAction
andChangeOverChainAction
actions and are moved tostartConsumerChain
andchangeoverChain
so we do not have to set the threshold in a multitude of other places. This was just part of small refactoring.