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

review: future proof appconsts SubtreeRootThreshold #2158

Closed
tuxcanfly opened this issue Jul 24, 2023 · 4 comments
Closed

review: future proof appconsts SubtreeRootThreshold #2158

tuxcanfly opened this issue Jul 24, 2023 · 4 comments
Labels
chore optional label for items that follow the `chore` conventional commit proposal item is not yet actionable and is suggesting a change that must first be agreed upon

Comments

@tuxcanfly
Copy link

tuxcanfly commented Jul 24, 2023

We should future proof these methods.

func SubtreeRootThreshold(version uint64) int {
        switch version {
        case v1.Version:
              return v1.SubtreeRootThreshold
        default:
             panic('invalid version for SubtreeRootThreshold')
        }
}

also doesn't hurt to have a unit test for these.

Originally posted by @MSevey in celestiaorg/celestia-openrpc#32 (comment)

@rootulp
Copy link
Collaborator

rootulp commented Jul 25, 2023

Note, there is an existing thread that discusses the pros and cons of a switch statement with a default panic.

@rootulp rootulp added chore optional label for items that follow the `chore` conventional commit proposal item is not yet actionable and is suggesting a change that must first be agreed upon and removed needs:triage labels Jul 25, 2023
@evan-forbes
Copy link
Member

per that thread, adding pacnics doesn't actually provide us with anything since comet is already checking during header verification. this means that in the application these can never get hit in the event that a chain updates and a consensus node is not aware of that upgrade.

there is almost all downside for core/app because if we forget to update a value, for example one that only gets hit once a year (like if we make a consensus breaking change to the mint module), then we will panic, halting the chain simply because we forgot to update or test a value.

similarly, other repos that import these values must do the same thing, where they check that the app version changed how they expected it to during header verification anyway. If they are doing that, then the same concept applies and they only open up a potential halt if there is a human error

@evan-forbes
Copy link
Member

if that makes sense @tuxcanfly and @MSevey then I think we can close this issue

@MSevey
Copy link
Member

MSevey commented Aug 8, 2023

fine to close

@MSevey MSevey closed this as completed Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore optional label for items that follow the `chore` conventional commit proposal item is not yet actionable and is suggesting a change that must first be agreed upon
Projects
None yet
Development

No branches or pull requests

4 participants