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

Remove special case in consumer addition/removal proposal handlers #458

Closed
2 tasks done
danwt opened this issue Nov 9, 2022 · 2 comments
Closed
2 tasks done

Remove special case in consumer addition/removal proposal handlers #458

danwt opened this issue Nov 9, 2022 · 2 comments
Assignees
Labels
scope: testing Code review, testing, making sure the code is following the specification. type: refactoring Code refactoring type: tech-debt Slows down development in the long run

Comments

@danwt
Copy link
Contributor

danwt commented Nov 9, 2022

Problem

The handlers unnecessarily (IMO) fork the execution by having two paths

if !ctx.BlockTime().Before(p.SpawnTime) {
// lockUbdOnTimeout is set to be false, regardless of what the proposal says, until we can specify and test issues around this use case more thoroughly
return k.CreateConsumerClient(ctx, p.ChainId, p.InitialHeight, false)
}
err := k.SetPendingConsumerAdditionProp(ctx, p)

if !ctx.BlockTime().Before(p.StopTime) {
return k.StopConsumerChain(ctx, p.ChainId, false, true)
}
k.SetPendingConsumerRemovalProp(ctx, p.ChainId, p.StopTime)

This doubles the number of code paths to be tested and reasoned about for little or no gain because all pending proposals are processed in BeginBlock.

Closing criteria

Remove the 'immediate' branches

if !ctx.BlockTime().Before(p.SpawnTime) {
// lockUbdOnTimeout is set to be false, regardless of what the proposal says, until we can specify and test issues around this use case more thoroughly
return k.CreateConsumerClient(ctx, p.ChainId, p.InitialHeight, false)
}

TODOs

  • Labels have been added for issue
  • Issue has been added to the ICS project

Detail

I can see some extremely minor benefit to ux, perhaps if a consumer has to be stopped in 'an emergency' or something.

Priority

This is low priority because everything seems to be working, although there might be odd yet-to-be solved things going on with the proposals (see #438)

@danwt
Copy link
Contributor Author

danwt commented Nov 9, 2022

cc @MSalopek

@danwt danwt moved this to Todo in Replicated Security Nov 9, 2022
@danwt danwt added scope: testing Code review, testing, making sure the code is following the specification. type: tech-debt Slows down development in the long run type: refactoring Code refactoring labels Nov 9, 2022
@danwt
Copy link
Contributor Author

danwt commented Nov 10, 2022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: testing Code review, testing, making sure the code is following the specification. type: refactoring Code refactoring type: tech-debt Slows down development in the long run
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants