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.

Sign up for GitHub

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

Jump to bottom

EPIC: Twilight (0.47) #13456

Closed
29 of 54 tasks
tac0turtle opened this issue Oct 5, 2022 · 16 comments
Closed
29 of 54 tasks

EPIC: Twilight (0.47) #13456

tac0turtle opened this issue Oct 5, 2022 · 16 comments
Assignees
@kocubinski @amaury1093 @alexanderbez @JeancarloBarrios @facundomedica @tac0turtle @julienrbrt @cool-develope @atheeshp @likhita-809
Labels
T:Epic Epics

Comments

@tac0turtle
Copy link
Member

tac0turtle commented Oct 5, 2022 •
edited by alexanderbez
Loading

Summary

This issue is meant to outline the next release of the sdk and QA required for all the changes.

Major Changes:

  • Introduction state changes for IS
  • Param module migration
    • params are handled by each module
    • introduction of a consensus param module to handle modifying tendermint consensus parameters
  • ABCI 1.0 (prepare & process proposal) & App side mempool
  • migrate from gogo/proto to cosmos/gogoproto
    • upgrade docs

Work Breakdown

Audit checklist

  • please copy to a markdown to follow while you walk through the code

  • 2 people should be assigned to each section

  • the auditors should work in silo and then at the end compare notes of concerns or changes they believe should happen

  • API audit

    • spec audit: check if the spec is complete.
    • Are Msg and Query methods and types well-named and organized?
    • Is everything well documented (inline godoc as well as /spec/ folder in module directory)
    • check the proto definition - make sure everything is in accordance to ADR-30 (at least 1 person, TODO assignee)
      • Check new fields and endpoints have the Since: cosmos-sdk 0.47 comment

  • Completeness audit, fully implemented with tests

    • Genesis import and export of all state
    • Query services
    • CLI methods
    • All necessary migration scripts are present (if this is an upgrade of existing module)

  • State machine audit

    • Read through MsgServer code and verify correctness upon visual inspection
    • Ensure all state machine code which could be confusing is properly commented
    • Make sure state machine logic matches Msg method documentation
    • Ensure that all state machine edge cases are covered with tests and that test coverage is sufficient (at least 90% coverage on module code)
    • Assess potential threats for each method including spam attacks and ensure that threats have been addressed sufficiently. This should be done by writing up threat assessment for each method. Specifically we should be paying attention to:
      • algorithmic complexity and places this could be exploited (ex. nested for loops)
      • charging gas complex computation (ex. for loops)
      • storage is safe (we don't pollute the state).
    • Assess potential risks of any new third party dependencies and decide whether a dependency audit is needed
    • Check correctness of simulation implementation if any

  • Audit Changelog against commit log, ensuring all breaking changes, bug fixes, and improvements are properly documented.

There is a small testnet running as well to test against. Endpoints have been shared in slack.

Release Documentation

This will be a living epic until twilight is released.

Previous version EPIC: #11096, #11362
@tac0turtle tac0turtle pinned this issue Oct 5, 2022
@alexanderbez
Copy link
Contributor

I'd like to propose removing IS from Twilight.

@tac0turtle
Copy link
Member Author

IS or LSM?

@alexanderbez
Copy link
Contributor

Sorry, LSM.

This was referenced Oct 17, 2022
Closed
Closed
@julienrbrt julienrbrt mentioned this issue Oct 19, 2022
Merged
19 tasks
@ethanfrey
Copy link
Contributor

ethanfrey commented Nov 4, 2022 •
edited
Loading

Is this the only issue tracking what is needed for 0.47 release?
Or do you need to close all these issues: https://github.com/cosmos/cosmos-sdk/labels/R%3ATwilight ?

I ask, as it seemed 0.47 was more of a stable cleanup, while most of the remaining issues are hefty refactorings, nice to haves, or docs.

Docs are not blocking a 0.47.0-alpha. Which of these are needed for release?

  • EPIC: Liquid staking  #13073 seems VERY big, and an additive change that could be added in eg 0.47.1 without affecting other modules.
  • EPIC: ABCI++ #12272 seems to need some larger rewrites to do it properly, and maybe best to put that into 0.48
  • The protobuf annotations Add amino JSON proto annotations #13407 and Require cosmos.msg.v1 annotations #13405 are coming from someone who is not developing client tools (or at least nothing widely used). I would check with @pyramation if those help him, as he maintains telescope, which also generates the protobuf bindings for CosmJS now, as well as for custom chains. As his code covers 90%+ of all protobuf client usage - I would only add what he finds essential.

Besides those, it would be basically ready for rc and proper testing. I think #13408 is key and giving time for an rc and other repos to adapt to it and run together on some public testnets would be great for a solid 0.47.0 release

This was referenced Nov 4, 2022
Closed
Closed
@robert-zaremba
Copy link
Collaborator

Why removing LSM? Isn't it ready? cc @alexanderbez

@pyramation
Copy link
Contributor

pyramation commented Nov 4, 2022 •
edited
Loading

  • The protobuf annotations Add amino JSON proto annotations #13407 and Require cosmos.msg.v1 annotations #13405 are coming from someone who is not developing client tools (or at least nothing widely used). I would check with @pyramation if those help him, as he maintains telescope, which also generates the protobuf bindings for CosmJS now, as well as for custom chains. As his code covers 90%+ of all protobuf client usage - I would only add what he finds essential.

Definitely let's include (if possible) Add amino JSON proto annotations #13407 — I think it just needs last final approval from @ValarDragon in feat: Add proto annotations for Amino JSON #13501

@alexanderbez
Copy link
Contributor

Why removing LSM? Isn't it ready? cc @alexanderbez

It still need review by core devs. We reviewed proto changes only IIRC.

This was referenced Nov 25, 2022
Merged
Closed
Merged
@likhita-809 likhita-809 mentioned this issue Nov 29, 2022
Closed
@atheeshp atheeshp mentioned this issue Nov 29, 2022
Merged
19 tasks
mergify bot pushed a commit that referenced this issue Nov 30, 2022
@atheeshp
chore: (x/mint) improve code cov (#14066)
e087566 
## Description

missed the test coverage in last audit.
ref: #13988 #13456 



---

### Author Checklist

*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.*

I have...

- [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed 
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
mergify bot pushed a commit that referenced this issue Nov 30, 2022
@atheeshp @mergify
chore: (x/mint) improve code cov (#14066)
84db39f 
## Description

missed the test coverage in last audit.
ref: #13988 #13456

---

### Author Checklist

*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.*

I have...

- [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)

(cherry picked from commit e087566)
tac0turtle pushed a commit that referenced this issue Nov 30, 2022
@mergify @atheeshp
chore: (x/mint) improve code cov (#14066) (#14094)
7217de3 
## Description

missed the test coverage in last audit.
ref: #13988 #13456

---

### Author Checklist

*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.*

I have...

- [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)

(cherry picked from commit e087566)

Co-authored-by: atheeshp <59333759+atheeshp@users.noreply.github.com>
@github-project-automation github-project-automation bot moved this to 📝 Todo in Cosmos-SDK Jan 2, 2023
@tac0turtle
Copy link
Member Author

closing this, we are waiting on ibc to tag a alpa tag to do a public testnet now

@github-project-automation github-project-automation bot moved this from 📝 Todo to 👏 Done in Cosmos-SDK Jan 24, 2023
@aaronc
Copy link
Member

aaronc commented Jan 24, 2023 •
edited
Loading

Let's just make sure the file descriptor bug fix (#14713) is tracked to go in before release

@alexanderbez
Copy link
Contributor

Let's just make sure the file descriptor bug fix (#14713) is tracked to go in before release

If this is a critical blocker, technically this issue should not have been closed until that's merged.

@aaronc
Copy link
Member

aaronc commented Jan 24, 2023

Just to restate the reason for considering it critical - fixing it will result in startup errors for chains with bad descriptors, IMHO better to not push that off to a patch release

@tac0turtle
Copy link
Member Author

its tracked as critical in the project board. we will still run a testnet with ibc and wasm so there is still time. We can reopen if needed.

@alexanderbez
Copy link
Contributor

Yeah sure, but the tracking issue epic shouldn't be closed until all critical items are addressed.

@aaronc
Copy link
Member

aaronc commented Jan 24, 2023 •
edited
Loading

Is there a reason we're not tracking releases with milestones now? IMHO it's pretty convenient for this sort of stuff

@tac0turtle
Copy link
Member Author

Reopening, we track open issues for a release with the label. In this case it wasn't added to the issue

@tac0turtle tac0turtle reopened this Jan 24, 2023
@github-project-automation github-project-automation bot moved this from 👏 Done to 📝 Todo in Cosmos-SDK Jan 24, 2023
@julienrbrt
Copy link
Member

#14713 has been closed.

@github-project-automation github-project-automation bot moved this from 📝 Todo to 👏 Done in Cosmos-SDK Feb 9, 2023
@tac0turtle tac0turtle unpinned this issue Feb 22, 2023
@tac0turtle tac0turtle removed this from Cosmos-SDK Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kocubinski kocubinski

@amaury1093 amaury1093

@alexanderbez alexanderbez

@JeancarloBarrios JeancarloBarrios

@facundomedica facundomedica

@tac0turtle tac0turtle

@julienrbrt julienrbrt

@cool-develope cool-develope

@atheeshp atheeshp

@likhita-809 likhita-809

Labels
T:Epic Epics
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests