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
chore: bump sdk to v1.20.0 #2860
chore: bump sdk to v1.20.0 #2860
Changes from all commits
20009fd
fc378bd
9536657
a6b82fc
856c42f
69f9d6a
9dec0bb
16e071a
fa90c36
e60aed5
71dd65a
6cb16da
1dae88c
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.
why is this no longer necessary?
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's done in the SDK instead
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 https://github.com/celestiaorg/cosmos-sdk/blob/18e768c968e05660e4ad0a7f4cc3ea8a33d5c993/baseapp/abci.go#L64
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.
Assuming it can be done in either location (app or SDK), why do it in the SDK where it has the risk of getting overwritten when we pull upstream changes?
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.
Because this behaviour is logically the responsibility of baseapp not app
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.
Then can it be upstreamed to cosmos/cosmos-sdk so it doesn't accidentally get reverted in celestiaorg/cosmos-sdk when we pull upstream changes?
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 switch statement has cases that fall through without any action or comment, which can be confusing. Specifically, the cases for
isSemVer
,latestVersion == "latest"
, andlen(latestVersion) == 7
do not have any code or comments. It's important to either implement the necessary logic or add comments explaining why no action is taken. This will improve the maintainability and readability of the code.Commitable suggestion
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.
good idea!