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

V0.45.13 proposal #176

Merged
merged 3 commits into from
Mar 17, 2023
Merged

V0.45.13 proposal #176

merged 3 commits into from
Mar 17, 2023

Conversation

nghuyenthevinh2000
Copy link
Contributor

@nghuyenthevinh2000 nghuyenthevinh2000 commented Mar 14, 2023

Summary of changes

No chain migration code is needed

There is only REST endpoints migration: https://github.com/cosmos/cosmos-sdk/blob/v0.45.13/docs/migrations/rest.md. This upgrade will break all current front - end.

Report of required housekeeping

  • Github issue OR spec proposal link
  • Wrote tests
  • Updated API documentation (client/lcd/swagger-ui/swagger.yaml)
  • Added a relevant changelog entry: clog add [section] [stanza] [message]

(FOR ADMIN) Before merging

  • Added appropriate labels to PR
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)
  • Confirm added tests are consistent with the intended behavior of changes
  • Ensure all tests pass

@fragwuerdig
Copy link
Collaborator

@inon-man @nghuyenthevinh2000: What implications has this for Mantlemint and FCD?

Copy link
Contributor

@ZaradarBH ZaradarBH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM buddy. Great work! :)

@nghuyenthevinh2000
Copy link
Contributor Author

nghuyenthevinh2000 commented Mar 15, 2023

@fragwuerdig
can I have your review and approval on this one?

Also, I have figured out the right way to modify the gas test. If you can check the gas test, you will see that it is very minimal

@fragwuerdig
Copy link
Collaborator

figured out the right way to modify the gas test. If you can check the gas test, you will see that it is very minimal

Do you have an idea what we need to modify in FCD so that we do not break the Infra?

@nghuyenthevinh2000
Copy link
Contributor Author

figured out the right way to modify the gas test. If you can check the gas test, you will see that it is very minimal

Do you have an idea what we need to modify in FCD so that we do not break the Infra?

No idea at the moment, but changes will happen to layer 2 and not layer 1

@faddat
Copy link
Contributor

faddat commented Mar 15, 2023

Hey I heard there was need of a review here. So normally I would totally lint this, before merging it.

If you're cool with waiting until tomorrow, then I'm happy to review. What I need to know in order to review this effectively, is the style and goal of this particular pull request.

@nghuyenthevinh2000 can you explain to me, what the goals are here, and how you intend to deal with WASM in this PR?

@faddat
Copy link
Contributor

faddat commented Mar 15, 2023

@fragwuerdig I think that it really makes sense to just assume that this is going to break FCD. I think that there are too many breaking changes to be able to easily advise on how not to make breaking changes.

@nghuyenthevinh2000 is correct to simply say that this will break front ends. Not much way around it.

@faddat
Copy link
Contributor

faddat commented Mar 15, 2023

I think that we should be using a later version of TMDb, that uses grocksdb instead of gorocksdb.

@faddat
Copy link
Contributor

faddat commented Mar 15, 2023

Tendermint 34.24 has a CPU griefing issue, and the minimum version that I consider safe is

GitHub.com/informalsystems/tendermint v0.34.26

@faddat
Copy link
Contributor

faddat commented Mar 15, 2023

Since we are updating the cosmos SDK, the minimum version that we should be updating to is v0.45.14

go.mod Show resolved Hide resolved
go.mod Show resolved Hide resolved
go.mod Show resolved Hide resolved
go.mod Show resolved Hide resolved
go.mod Show resolved Hide resolved
go.mod Show resolved Hide resolved
go.mod Show resolved Hide resolved
go.mod Show resolved Hide resolved
go.mod Show resolved Hide resolved
go.mod Show resolved Hide resolved
go.mod Show resolved Hide resolved
@faddat
Copy link
Contributor

faddat commented Mar 15, 2023

Since the changes here aren't gigantic, in order to gain the benefit of using the linter, and the additional tests and error checks that are in the linted branch, I would merge this branch into this one or merge that branch into this one.

@faddat
Copy link
Contributor

faddat commented Mar 15, 2023

Given how many changes are going to occur on the front end side, I approve of bluntly stating that this will break front ends. I also may recommend that this just be refactored to use 47. Mainly you would be making changes to the governance proposal handlers.

@faddat
Copy link
Contributor

faddat commented Mar 15, 2023

I merged this branch into the linted one. This makes it easier to find issues. Remember to turn on golangci-lint in your editor.

@faddat faddat mentioned this pull request Mar 15, 2023
8 tasks
@faddat
Copy link
Contributor

faddat commented Mar 15, 2023

Basically I don't approve this PR, until:

  • it has been linted

  • it no longer uses vulnerable software

  • the replace section of go.mod is limited more effectively

  • Linting and Code Scanning #178 should be merged here before this is considered for merge to main.

@ZaradarBH
Copy link
Contributor

ZaradarBH commented Mar 15, 2023

Hey I heard there was need of a review here. So normally I would totally lint this, before merging it.

If you're cool with waiting until tomorrow, then I'm happy to review. What I need to know in order to review this effectively, is the style and goal of this particular pull request.

@nghuyenthevinh2000 can you explain to me, what the goals are here, and how you intend to deal with WASM in this PR?

Thank you for providing feedback. The more the merrier, just lets keep it civil and remember we all have the same goal here <3

We intend to leave the TFL WASM customization in place for the 2.0 upgrade, since we want to make sure we can have complete focus on the WASM migration process that L2 apps will have to go thru. Thus we will revert to the canonical Cosmos SDK WASM module in 2.1.

The 2.0 / 2.1 upgrades will break the FCD but as such I believe that it will mainly be the protobuf message contracts so it will be fairly simple to upgrade the FCD TypeScript dependencies and do whatever tweaking is needed once we have the new message contract dependency in place (terra.proto / terra.js). Terra-station, desktop, etc will also be affected by the changes to the message contracts. But the 0.45.x upgrade is largely just extensions to existing msg schemas so it should be mainly about rewiring code, thus it should not take to long to fix.

Regarding this comment:

"Tendermint 34.24 has a CPU griefing issue, and the minimum version that I consider safe is

GitHub.com/informalsystems/tendermint v0.34.26"

As I can see it TM 34.26 is really just "pre-stage-CometBFT", since its released by informalsystems and not the official tendermint release? Either way, I tried bumping 0.45.13 to use 34.26 but it seemed to introduce some breakage due to some issues related to github.com/gogo/protobuf v1.3.3 and ultimately a missing replace statement for informals TM version. However I cannot find any reference to the "CPU griefing issue" in their v0.34.26 CHANGELOG section, do you have a link for some info on that particular issue?

It should be safe to bump to Cosmos SDK v0.45.14, which has a DDoS patch of some sort. However I think we can do that safely in the context of your clean up PR and release it as part of v2.1 or 2.2 (which might also be named 3.0 depending on the changes needed to adopt CometBFT). All of this is granted that you downgrade to Go 1.18, as we don't have the resources (aka manpower) at the moment to safely ensure all validators update their go installations and dealing with potential "unknown, unknowns", which we most likely will have to do if we make this a hard requirement.

@ZaradarBH ZaradarBH added enhancement New feature or request in scope Work approved by the community labels Mar 16, 2023
@ZaradarBH ZaradarBH modified the milestones: Oracle Feeder - v1.0.0, Core - v2.0.0 Mar 16, 2023
x/wasm/types/gas_meter.go Outdated Show resolved Hide resolved
x/wasm/types/gas_meter_test.go Outdated Show resolved Hide resolved
@ZaradarBH ZaradarBH merged commit 272ec36 into main Mar 17, 2023
@fragwuerdig fragwuerdig deleted the v0.45.13-proposal branch May 25, 2023 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request in scope Work approved by the community
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants