feat(fork): restricts to post genesis transfer heights only #1600
Conversation
danwt
changed the title
| if !k.rollappKeeper.ForkLatestAllowed(ctx, seq.RollappId) { | ||
| return nil, gerrc.ErrFailedPrecondition.Wrap("rotation could cause fork before genesis transfer") | ||
| } |
There was a problem hiding this comment.
eagerly prevent rotation when it would end up in a fork before genesis transfer
better ux and prevents rollapp getting completely broken
There was a problem hiding this comment.
why u check it on the notice period start?
I think it will male more sense, to check on rotation start (notice period ends), in case nextProposer is sentinel
| // is the rollback fork going to violate assumptions? | ||
| func (k Keeper) ForkAllowed(ctx sdk.Context, rollapp string, lastValidHeight uint64) bool { | ||
| ra := k.MustGetRollapp(ctx, rollapp) | ||
| return 0 < ra.GenesisState.TransferProofHeight && ra.GenesisState.TransferProofHeight <= lastValidHeight |
There was a problem hiding this comment.
missing true on ra.GenesisState.TransferProofHeight == 0
There was a problem hiding this comment.
we're not gonna allow to fork before genesis bridge period (regardless of if ibc opened or not) to avoid now edge cases.
There was a problem hiding this comment.
isn't it the use case this PR tries to solve?
we want to allow fork before genesis bridge as well
we just want to avoid rollback from post GB to pre GB
There was a problem hiding this comment.
No @mtsitrin we are not
| if !k.rollappKeeper.ForkLatestAllowed(ctx, seq.RollappId) { | ||
| return nil, gerrc.ErrFailedPrecondition.Wrap("rotation could cause fork before genesis transfer") | ||
| } |
There was a problem hiding this comment.
why u check it on the notice period start?
I think it will male more sense, to check on rotation start (notice period ends), in case nextProposer is sentinel
|
why u check it on the notice period start? I disagree that's way more complicated and no ux improvement |
Description
Closes
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow-up issues.
PR review checkboxes:
I have...
Unreleasedsection inCHANGELOG.mdgodoccommentsSDK Checklist
maptime.Now()sendCoinand notSendCoinsFull security checklist here
For Reviewer:
After reviewer approval: