search

osmosis-labs / osmojs

OsmosJS makes it easy to compose and broadcast Osmosis and Cosmos messages
https://cosmology.zone/products/osmojs
Apache License 2.0
63 stars 32 forks source link

Need an upgrade for ibc-go proto #33

Closed giorgionocera closed 1 year ago

giorgionocera commented 1 year ago

There seems to be a misalignment between the latest cosmos proto and the osmojs ones for the IBC transactions.

As you can see, the proto file available on the cosmos repo in the ibc-go folder includes the info for the IBC memo parameter: https://github.com/cosmos/ibc-go/blob/10324f5bf6840892cd7bba8a40231fc52a8b1745/proto/ibc/applications/transfer/v2/packet.proto#L5-L21

As you can see here: https://github.com/osmosis-labs/osmojs/blob/59d389d695ea9325310dc014a52ef314bc3e55b0/packages/osmojs/proto/ibc/applications/transfer/v2/packet.proto#L5-L19

osmojs seems to be not yet updated 😬, nor at its last version: https://github.com/osmosis-labs/osmojs/blob/59d389d695ea9325310dc014a52ef314bc3e55b0/packages/osmojs/proto/ibc/applications/transfer/v2/packet.proto#L5-L19

On the other hand, it seems that cosmjs already provide such a feature: https://github.com/confio/cosmjs-types/blob/6e3448a046b86f4448d577a73edf8f2229e8ad61/src/ibc/applications/transfer/v2/packet.ts#L11-L26

Moreover, could you please construct the MessageComposer as you did for the IBC transfer message?

DavideSegullo commented 1 year ago

Hello, I also suggest to update the ibc-go v1 transfer,because they added the memo field: https://github.com/cosmos/ibc-go/blob/10324f5bf6840892cd7bba8a40231fc52a8b1745/proto/ibc/applications/transfer/v1/tx.proto#L41

As you can see here, the memo field is missing: https://github.com/osmosis-labs/osmojs/blob/59d389d695ea9325310dc014a52ef314bc3e55b0/packages/osmojs/proto/ibc/applications/transfer/v1/tx.proto#L40

Thank you!

pyramation commented 1 year ago

working on this now, will be using git submodules to get the latest. Had to make some updates to telescope for some new syntax that is popping up in here.

pyramation commented 1 year ago

tracking https://github.com/osmosis-labs/osmojs/pull/35

pyramation commented 1 year ago

some amino messages have changed, not sure if this is the correct version of the SDK

-   "type": "cosmos-sdk/MsgVote",
+   "type": "cosmos-sdk/v1/MsgVote",
-   "type": "cosmos-sdk/MsgDeposit",
+   "type": "cosmos-sdk/v1/MsgDeposit",
-   "type": "cosmos-sdk/MsgWithdrawValidatorCommission",
+   "type": "cosmos-sdk/MsgWithdrawValCommission",
nicolaslara commented 1 year ago

The new amino names seem correct. It matches both the osmosis sdk fork and upstream: https://github.com/osmosis-labs/cosmos-sdk/blob/6ed904cdb75fe08a9310e7df49eaf9c5b2b64692/proto/cosmos/distribution/v1beta1/tx.proto#L62 https://github.com/cosmos/cosmos-sdk/blob/59e5ca20b65a43896f4704f879f78358b060bc58/proto/cosmos/distribution/v1beta1/tx.proto#L101

This seems to have been introduced in November (https://github.com/cosmos/cosmos-sdk/pull/13501) was the previous generation of osmojs older than that?

pyramation commented 1 year ago

ok cool, I've upgraded and published

osmojs@15.2.0
DavideSegullo commented 1 year ago

Thank you! But it seems that "memo" fields are still missing, or am I missing something?

nicolaslara commented 1 year ago

@DavideSegullo the memo seems to be there: https://github.com/osmosis-labs/osmojs/blob/osmojs%4015.2.0/packages/osmojs/src/codegen/ibc/applications/transfer/v2/packet.ts#L22

Are you using 15.2.0? how are you using it?

DavideSegullo commented 1 year ago

Oh sorry, the linter probably hadn't updated and was still showing me the old type definitions, now it seems good, I'll give it a try! Thank you for your support!

DavideSegullo commented 1 year ago

Everything seems okay to me, I even tested with transfer messages using the memo!

giorgionocera commented 1 year ago

Yup, thank you, guys! 🙏 I think we're done with this issue. It can be closed.

giorgionocera commented 1 year ago

We only saw that amino is not working with this message: it seems that the encoded message does not have the memo. But we do not know if it is an issue due to this implementation. At the moment we are using the "sign direct".

giorgionocera commented 1 year ago

My fault 😬 amino types are working. The signer called by cosmos-kit refers to not updated amino types. In fact, if you overwrite the default ones with the ones exported by osmojs, everything works smoothly.

I hope this could help!