Skip to content
This repository has been archived by the owner on Oct 16, 2024. It is now read-only.

Fix for Cosmos-SDK 0.44 #32

Closed
wants to merge 26 commits into from
Closed

Fix for Cosmos-SDK 0.44 #32

wants to merge 26 commits into from

Conversation

ctrl-Felix
Copy link

After providing a quick and dirty fix last time I now found some time to take a proper approach to this and this should finally fix the compability. This pull request includes:

I think this should be it. All in all the biggest gain is that cosmospy will be working again.

Copy link
Owner

@hukkin hukkin left a comment

Choose a reason for hiding this comment

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

Thanks a lot! 🥳

I'll do a proper review in the upcoming days. This is just a drive-by note about the extra dependencies.

Also, if you'd be kind enough to provide the commands/actions that you executed to autogenerate the protobuf files that'd be great!

pyproject.toml Outdated Show resolved Hide resolved
@ctrl-Felix
Copy link
Author

ctrl-Felix commented Apr 27, 2022

Thank you very much for your quick reply 😄
I used the protobuf compiler from here with following command:
Execute in the proto folder
protoc --python-out=<absolute path to interfaces dir> file.proto

Regarding the requests. I added two commits which removes the code snipped which used the requests dependency. (The second one is because I forgot to remove the actual dependency...)

Adding requests could be useful for helper functions tho. Like a function to get the account sequence and account number which are only accessible through a api call.

the correct methods for the rpc are: broadcast_tx_sync, broadcast_tx_async and broadcast_tx_commit. The ones for the api are slightly different
The default sync mode wasn't correct as it was a default api sync mode but it's better to use rpc sync modes.
@ctrl-Felix
Copy link
Author

ctrl-Felix commented Apr 30, 2022

Sorry adding for these commits later on. I just saw two things while going through the code once again:

  • The first one was that I mesed up the broadcast modes for the api and rpc call. While the rpc refers to it as method it wants bradcast_tx_sync as method. The api refers to it as mode and therefore wants it as BROADCAST_MODE_SYNC
  • Second one was that I was using protoc 3.6.1 which worked. But it's 4 years old. I took the recent version now to compile the files and the ward tests go through. However there seeom to be a coverage issue or whatever in the automated github checks. I am not yet sure what that means

src/cosmospy/_typing.py Outdated Show resolved Hide resolved
src/cosmospy/_typing.py Outdated Show resolved Hide resolved
src/cosmospy/_transaction.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/cosmospy/_transaction.py Outdated Show resolved Hide resolved
src/cosmospy/_transaction.py Outdated Show resolved Hide resolved
ctrl-Felix and others added 5 commits May 2, 2022 22:35
Co-authored-by: Taneli Hukkinen <3275109+hukkin@users.noreply.github.com>
Co-authored-by: Taneli Hukkinen <3275109+hukkin@users.noreply.github.com>
Co-authored-by: Taneli Hukkinen <3275109+hukkin@users.noreply.github.com>
Co-authored-by: Taneli Hukkinen <3275109+hukkin@users.noreply.github.com>
@ctrl-Felix
Copy link
Author

ctrl-Felix commented May 2, 2022

I went through everything now I think. I have to admit I am not 100% happy by removing the get_rpc_pushable and get_api_pushable methods, especially as this method was implemented before too.

Note: With the explanation I would say I am then 100% happy with removing it 😏

Copy link
Owner

@hukkin hukkin left a comment

Choose a reason for hiding this comment

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

Let's revert this unless there's a good reason for the change?

src/cosmospy/_transaction.py Outdated Show resolved Hide resolved
src/cosmospy/_transaction.py Outdated Show resolved Hide resolved
ctrl-Felix and others added 4 commits May 3, 2022 00:43
Co-authored-by: Taneli Hukkinen <3275109+hukkin@users.noreply.github.com>
Co-authored-by: Taneli Hukkinen <3275109+hukkin@users.noreply.github.com>
Copy link
Owner

@hukkin hukkin left a comment

Choose a reason for hiding this comment

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

Could you sync the proto files (and generate .py files) with what's up-to-date in https://github.com/cosmos/cosmos-sdk/tree/main/proto ?

src/cosmospy/_transaction.py Outdated Show resolved Hide resolved
@ctrl-Felix
Copy link
Author

Sorry for my late reply. I will do that, but I modified some of them to remove unused dependencies. The latest proto files include already stuff that will be introduced with cosmo ssdk 0.46 or transaction types we don't need but which need other dependencies

@ctrl-Felix
Copy link
Author

Here are the sources for the new proto files:

I removed the functions for MultiSig as well as other transaction types and only took MsgSend to keep the dependencies as low as possible

Changes I made to every .proto file:

  • Fix the imports to the dir structure in cosmospy

Changes I made to every compiled proto file:

@ctrl-Felix
Copy link
Author

Hi @hukkin
I just wanted to ask if you had time to check the new commits

@ctrl-Felix ctrl-Felix closed this Jun 28, 2022
@ctrl-Felix ctrl-Felix mentioned this pull request Jun 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants