add annotations for client tooling and codegen #3782

pyramation · 2022-12-18T00:52:24Z

What is the purpose of the change

This adds new annotations used by codegen tools to create encoders/decoders that can manage the google.protobuf.Any types. This change has many (positive) implications:

RPC calls get properly decoded

Currently, when you do an RPC call, for example,

const request: QueryPoolsRequest;
const result = await osmosis.gamm.v1beta1.pools(request);

In the current system, you get back results that just have a bunch of encoded bytes (not very useful for FE devs):

{
  "pools": [
    {
        "typeUrl": "/osmosis.gamm.v1beta1.Pool",
        "value": <a bunch of bytes>
    },
    {
        "typeUrl": "/osmosis.gamm.v1beta1.Pool",
        "value": <a bunch of bytes>
    },
    ...
    {
        "typeUrl": "/osmosis.gamm.poolmodels.stableswap.v1beta1.Pool",
        "value": <a bunch of bytes>
    }
  ]
}

Leveraging the implements_interface and accepts_interface, we can now derive and generate decoders, in this case, the pools:

export const PoolI_InterfaceDecoder = (input: _m0.Reader | Uint8Array): Pool1 | Pool2 | Any => {
  const reader = input instanceof _m0.Reader ? input : new _m0.Reader(input);
  const data = Any.decode(reader, reader.uint32());

  switch (data.typeUrl) {
    case "/osmosis.gamm.v1beta1.Pool":
      return Pool1.decode(data.value);

    case "/osmosis.gamm.poolmodels.stableswap.v1beta1.Pool":
      return Pool2.decode(data.value);

    default:
      return data;
  }
};

This will now give us the expected API result when fetching pools (and will also close issues like this one and make people like Larry0x happy)

Messages with nested messages/protos and their amino encoders for ledger support

Additionally, we can create encoders for toAmino/fromAmino that leverage interfaces with proper annotations. Here is an example for proposals that multiplex on content using the Content interface:

This can enable a new class of dApps built on osmosis. Currently developers have to manually construct all of these encodings.

Changes required

importing `amino/amino.proto`

we need to be able to import amino/amino.proto which currently exists in the cosmos/cosmos-sdk
issue in osmosis-labs/cosmos-sdk is here

Notes (and nice to haves)

Missing codec registrations

These are missing amino registrations (via RegisterConcrete):

MsgExtendLockup
MsgForceUnlock
MsgRedelegateValidatorSet

These are missing amino registration (via RegisterConcrete), but registered via RegisterProposalTypeCodec:

ReplacePoolIncentivesProposal (osmosis/ReplacePoolIncentivesProposal)
SetProtoRevAdminAccountProposal
SetProtoRevEnabledProposal

Conventions

convention for `Register*` names (amino/proposal/etc)

should we have a convention for Register* names? I noticed some discrepancies:

osmosis/SetSuperfluidAssetsProposal for RegisterProposalTypeCodec vs osmosis/set-superfluid-assets-proposal for RegisterConcrete
osmosis/UpdatePoolIncentivesProposal is used for BOTH RegisterProposalTypeCodec and RegisterConcrete

interface conventions

determine how we're going to annotate interfaces

Once we resolve that conversation, we can determine if we need to do this

-  option (cosmos_proto.implements_interface) = "cosmos.gov.v1beta1.Content";
+ option (cosmos_proto.implements_interface) = "Content";

Brief Changelog

imports amino/amino.proto (which means this PR needs to be rebased BEFORE merge, issue here)
adds interface annotations
adds amino annotations

Testing and Verifying

This change added tests and can be verified as follows:

should NOT affect Go compilation (to my knowledge)
was tested locally for tranpilation/codegen with Telescope

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes? NO — not for Go or Cosmos SDK side of things.

This reverts commit 72b34ce.

faddat · 2022-12-18T07:52:02Z

I'm of the opinion that it is important to make Larry happy!

You mentioned that this would be aided by adding annotations to the osmosis-sdk, right?

pyramation · 2022-12-18T08:36:49Z

I'm of the opinion that it is important to make Larry happy!

You mentioned that this would be aided by adding annotations to the osmosis-sdk, right?

yep! this depends on importing amino/amino.proto, which is a new proto that exists in the cosmos/cosmos-sdk. I've raised an issue in osmosis-labs/cosmos-sdk here

ValarDragon

Looks great to me! I'll do the proto builds

ValarDragon · 2022-12-21T20:09:46Z

ah wait no, we need to update our SDK to have amino.proto first.

pyramation · 2022-12-22T00:17:36Z

ah wait no, we need to update our SDK to have amino.proto first.

yep, issue documented here, will comment over there now osmosis-labs/cosmos-sdk#400

mattverse

Do we have any test that has been done to ensure that this way of amino encoding / decoding ensures compatibility with the old way of handling amino?

Otherwise LGTM, but lets hold merging until osmosis-labs/cosmos-sdk#400 has been pushed forward

pyramation · 2023-01-03T00:39:23Z

Do we have any test that has been done to ensure that this way of amino encoding / decoding ensures compatibility with the old way of handling amino?

There is not a test, but as far as I understand it, this is only annotations for fields, option (amino.name). No Go code should be affected. Maybe @AmauryM or @tac0turtle can comment more about the impact these annotations have as they're aware of the new proto annotations.

Otherwise LGTM, but lets hold merging until osmosis-labs/cosmos-sdk#400 has been pushed forward

Cool!

tac0turtle · 2023-01-03T08:39:27Z

this will not effect go code since the generator doesn't account for it. It will be part of the proto descriptor that clients can use though

github-actions · 2023-01-18T00:02:16Z

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you!

pyramation · 2023-01-18T00:12:40Z

let's not mark this as stale or old, thanks!

github-actions · 2023-02-02T00:02:09Z

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you!

github-actions · 2023-02-17T00:02:13Z

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you!

pyramation · 2023-02-22T04:42:13Z

let me know if we can make this PR happen, or if I need to rebase and make a new one, thanks!

github-actions · 2023-03-09T00:02:16Z

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you!

pyramation · 2023-03-15T00:27:51Z

hey @mattverse — the last PR adds the amino file, right? Which means we likely can add this PR soon.

mattverse · 2023-03-15T05:59:54Z

hey @pyramation yeah! working on porting your changes over right now

mattverse · 2023-03-15T06:19:10Z

@pyramation Would you mind if I close this PR and open a new PR that includes these changes? Need some additional updates in go.mod and compile protobuf files as well

github-actions · 2023-03-30T00:02:18Z

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you!

pyramation · 2023-03-30T19:49:17Z

@pyramation Would you mind if I close this PR and open a new PR that includes these changes? Need some additional updates in go.mod and compile protobuf files as well

whatever works! sounds good!

mattverse · 2023-04-06T05:36:15Z

@pyramation sweet! Closing this in favor of #4629

add annotations for client tooling and codegen

476ab3d

pyramation mentioned this pull request Dec 18, 2022

new proto annotations exist upstream osmosis-labs/cosmos-sdk#400

Closed

pyramation added 2 commits December 17, 2022 17:53

simple interface namespace

72b34ce

Revert "simple interface namespace"

2a762f6

This reverts commit 72b34ce.

ValarDragon approved these changes Dec 21, 2022

View reviewed changes

mattverse approved these changes Jan 2, 2023

View reviewed changes

github-actions bot added the Stale label Jan 18, 2023

github-actions bot removed the Stale label Jan 19, 2023

github-actions bot added the Stale label Feb 2, 2023

mattverse removed the Stale label Feb 2, 2023

github-actions bot added the Stale label Feb 17, 2023

github-actions bot removed the Stale label Feb 23, 2023

github-actions bot added the Stale label Mar 9, 2023

p0mvn removed the Stale label Mar 13, 2023

mattverse mentioned this pull request Mar 15, 2023

Add amino proto annotations #4629

Merged

github-actions bot added the Stale label Mar 30, 2023

github-actions bot removed the Stale label Mar 31, 2023

Merge branch 'osmosis-labs:main' into cosmology/annotations

ac133fd

mattverse closed this Apr 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add annotations for client tooling and codegen #3782

add annotations for client tooling and codegen #3782

pyramation commented Dec 18, 2022 •

edited

Loading

faddat commented Dec 18, 2022

pyramation commented Dec 18, 2022 •

edited

Loading

ValarDragon left a comment

ValarDragon commented Dec 21, 2022

pyramation commented Dec 22, 2022

mattverse left a comment

pyramation commented Jan 3, 2023 •

edited

Loading

tac0turtle commented Jan 3, 2023

github-actions bot commented Jan 18, 2023

pyramation commented Jan 18, 2023

github-actions bot commented Feb 2, 2023

github-actions bot commented Feb 17, 2023

pyramation commented Feb 22, 2023

github-actions bot commented Mar 9, 2023

pyramation commented Mar 15, 2023

mattverse commented Mar 15, 2023

mattverse commented Mar 15, 2023 •

edited

Loading

github-actions bot commented Mar 30, 2023

pyramation commented Mar 30, 2023

mattverse commented Apr 6, 2023

add annotations for client tooling and codegen #3782

add annotations for client tooling and codegen #3782

Conversation

pyramation commented Dec 18, 2022 • edited Loading

What is the purpose of the change

RPC calls get properly decoded

Messages with nested messages/protos and their amino encoders for ledger support

Changes required

importing amino/amino.proto

Notes (and nice to haves)

Missing codec registrations

Conventions

convention for Register* names (amino/proposal/etc)

interface conventions

Brief Changelog

Testing and Verifying

Documentation and Release Note

faddat commented Dec 18, 2022

pyramation commented Dec 18, 2022 • edited Loading

ValarDragon left a comment

Choose a reason for hiding this comment

ValarDragon commented Dec 21, 2022

pyramation commented Dec 22, 2022

mattverse left a comment

Choose a reason for hiding this comment

pyramation commented Jan 3, 2023 • edited Loading

tac0turtle commented Jan 3, 2023

github-actions bot commented Jan 18, 2023

pyramation commented Jan 18, 2023

github-actions bot commented Feb 2, 2023

github-actions bot commented Feb 17, 2023

pyramation commented Feb 22, 2023

github-actions bot commented Mar 9, 2023

pyramation commented Mar 15, 2023

mattverse commented Mar 15, 2023

mattverse commented Mar 15, 2023 • edited Loading

github-actions bot commented Mar 30, 2023

pyramation commented Mar 30, 2023

mattverse commented Apr 6, 2023

pyramation commented Dec 18, 2022 •

edited

Loading

importing `amino/amino.proto`

convention for `Register*` names (amino/proposal/etc)

pyramation commented Dec 18, 2022 •

edited

Loading

pyramation commented Jan 3, 2023 •

edited

Loading

mattverse commented Mar 15, 2023 •

edited

Loading