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

[Cosmos] Revisit the encoding limitation of the newer version of cosmos #1033

Open
12 tasks
Olshansk opened this issue Jan 17, 2025 · 9 comments
Open
12 tasks
Assignees
Labels
on-chain On-chain business logic protocol General core protocol related changes tooling Tooling - CLI, scripts, helpers, off-chain, etc...

Comments

@Olshansk
Copy link
Member

Olshansk commented Jan 17, 2025

Objective

Understand what limitations the newer version of Cosmos imposes on us and adapt appropriately.

Origin Document

Image

Goals

  • Read, learn and understand the direction of the Cosmos SDK when it comes protobuf encoding
  • Follow best practices in how we are designing the structures (i.e. onchain state) and interfaces (gRPC queries & transactions) in poktroll
  • Prepare for the v0.52, v0.2 and beyond

Deliverables

  • Review the Zero Copy Encoding document from the Cosmos SDK:
  • API (gRPC) Quries & Transactions
  • Structures - Evaluate how we need to change any of our onchain structures to comply with:
    • Floats not being a supported scalar
    • Maps not being supported
    • Limitation of not having new fields
      The biggest change is that it will be invalid to add a new field to an existing message and a breaking change detector will need to be created which augments [buf breaking](https://docs.buf.build/breaking/overview) to detect this.
      
      The reasons for this are two-fold:
      
      1) from an API compatibility perspective, adding a new field to an existing message is actually a state machine breaking change which in [ADR 020](https://docs.cosmos.network/v0.52/build/architecture/adr-020-protobuf-transaction-encoding) required us to add an unknown field detector. Furthermore, in [ADR 054](https://docs.cosmos.network/v0.52/build/architecture/adr-054-semver-compatible-modules) this "feature" of protobuf poses one of the biggest problems for correct forward compatibility between different versions of the same module. 2) not allowing new fields in existing messages makes the generated code in languages like Rust (which is currently our highest priority target), much simpler and more performant because we can assume a fixed size struct gets allocated. If new fields can be added to existing messages, we need to encode the number of fields into the message and then do runtime checks. So this both increases memory layers and requires another layout of indirection. With the encoding proposed below, "plain old Rust structs" (used with some special field types) can be used.
      
      Instead of adding new fields to existing messages, APIs can add new messages to existing packages or create new packages with new versions of the messages. Also, we are not restricting the addition of cases to oneofs or values to enums. All of these cases are easier to detect at runtime with standard switch statements than the addition of new fields.
      

Non-goals / Non-deliverables

  • Forking the Cosmos SDK
  • Creating our own encoding rules / best practices
  • Trying to make the Cosmos SDK change based on our own opinions

General deliverables

  • Comments: Add/update TODOs and comments alongside the source code so it is easier to follow.
  • Testing: Add new tests (unit and/or E2E) to the test suite.
  • Makefile: Add new targets to the Makefile to make the new functionality easier to use.
  • Documentation: Update architectural or development READMEs; use mermaid diagrams where appropriate.

Creator: @Olshansk
Co-Owners: @red-0ne @bryanchriswhite

@Olshansk Olshansk added on-chain On-chain business logic protocol General core protocol related changes tooling Tooling - CLI, scripts, helpers, off-chain, etc... labels Jan 17, 2025
@Olshansk Olshansk added this to Shannon Jan 17, 2025
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Shannon Jan 17, 2025
@Olshansk Olshansk self-assigned this Jan 17, 2025
@Olshansk Olshansk moved this from 📋 Backlog to 🔖 Ready in Shannon Jan 17, 2025
@Olshansk
Copy link
Member Author

For reference, here is the error that forced us to move rev_share from float to uint64: https://github.com/pokt-network/poktroll/actions/runs/12832899448/job/35787150432?pr=1028

Image

@Olshansk
Copy link
Member Author

Quick update here.

@tac0turtle from Binary Builders mentioned that You can use maps but you just need to be sure to sort.

Also, looks like @johnletey from Noble already looked into adding maps into autocli: cosmos/cosmos-sdk@dd3d264

@bryanchriswhite @red-0ne I'm going to leave a handful of TODOs in #1028 as we navigate around this while moving forward, but just wanted to communicate the situation, status and learnings as they keep coming in.

@Olshansk
Copy link
Member Author

Capturing some other parts of the conversation for future reference

Image

@johnletey
Copy link

@Olshansk I've worked with the Cosmos SDK team to get my commit linked above backported into v0.50.x

Please bump cosmossdk.io/client/v2 to v2.0.0-beta.8, which should automatically include a bump of cosmossdk.io/x/tx to v0.13.8

@okdas okdas self-assigned this Feb 7, 2025
@Olshansk
Copy link
Member Author

Olshansk commented Feb 10, 2025

Thanks @johnletey!

@okdas from our team is going to handle this so I'll follow up then.


Unrelated - lmk if you're going to be at ETH Denver. Would love to talk about getting native USDC payments for Pocket Network's Shannon upgrade in place!

@johnletey
Copy link

lmk if you're going to be at ETH Denver.

I will be! What is the best place to get in touch with you to coordinate!

@Olshansk Olshansk assigned red-0ne and unassigned Olshansk and okdas Feb 11, 2025
@Olshansk
Copy link
Member Author

Olshansk commented Feb 11, 2025

I will be! What is the best place to get in touch with you to coordinate!

🔥

Ping me on telegram: https://t.me/@dolshansky

okdas added a commit that referenced this issue Feb 14, 2025
…uber`'s fork (#1069)

## Summary

Bumps `cosmossdk.io/client/v2` to `v2.0.0-beta.8`.

As a side effect, we had to change `mockgen` to uber's fork and switch
to `go run go.uber.org/mock/mockgen` instead of installing `mockgen`
binary.

## Issue

- Issue: #1033


## Type of change

Select one or more from the following:

- [x] New feature, functionality or library
- [ ] Consensus breaking; add the `consensus-breaking` label if so. See
#791 for details
- [ ] Bug fix
- [x] Code health or cleanup
- [x] Documentation
- [ ] Other (specify)

## Sanity Checklist

- [x] I have updated the GitHub Issue `assignees`, `reviewers`,
`labels`, `project`, `iteration` and `milestone`
- [x] For docs, I have run `make docusaurus_start`
- [x] For code, I have run `make go_develop_and_test` and `make
test_e2e`
- [x] For code, I have added the `devnet-test-e2e` label to run E2E
tests in CI
- [ ] For configurations, I have update the documentation
- [ ] I added TODOs where applicable
@okdas
Copy link
Member

okdas commented Feb 15, 2025

@Olshansk @red-0ne anything else needs to be done here, or bumping the cosmossdk.io/client/v2 dependency was enough?

@Olshansk
Copy link
Member Author

@okdas Everything seems to be working and updated in #1086.

Thanks @johnletey!

@Olshansk Olshansk moved this from 🔖 Ready to 👀 In review in Shannon Feb 20, 2025
@Olshansk Olshansk self-assigned this Feb 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
on-chain On-chain business logic protocol General core protocol related changes tooling Tooling - CLI, scripts, helpers, off-chain, etc...
Projects
Status: 👀 In review
Development

No branches or pull requests

4 participants