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

GetSignBytes broken for IBC messages #7638

Closed
jackzampolin opened this issue Oct 23, 2020 · 9 comments
Closed

GetSignBytes broken for IBC messages #7638

jackzampolin opened this issue Oct 23, 2020 · 9 comments
Assignees
Labels

Comments

@jackzampolin
Copy link
Member

The recent codec changes broke the SubModuleCdcs that are used to GetSignBytes for IBC messages. There is a test that reproduces this issue in #7637

@clevinson
Copy link
Contributor

@jackzampolin can you link to which recent codec changes you're referring to? Lot of pieces moving right now... Would be great if there's a specific PR that you know caused this issue.

@jackzampolin
Copy link
Member Author

The last PR that touched the code that is in 0.40.0-rc0 but not in master is #7086

@aaronc
Copy link
Member

aaronc commented Oct 23, 2020

IBC doesn't support amino at all so GetSignBytes is meaningless and should be considered basically deprecated.

@aaronc
Copy link
Member

aaronc commented Oct 23, 2020

Ibc GetSignBytes methods should probably all panic

@jackzampolin
Copy link
Member Author

@aaronc well they do! Maybe I need to migrate to another signing method.

@colin-axner
Copy link
Contributor

I think this is from the relayer feature of bundling relay messages by message size. Is there another way we can determine the bytes of a message easily? If not, I'd propose just removing the feature. We can limit relayed transactions by the number of messages which is sufficient and probably more useful imo

@colin-axner
Copy link
Contributor

colin-axner commented Oct 23, 2020

I think this fix should work for the relayer.

Maybe we should change the GetSignBytes function in the SDK to explicitly panic? We could probably remove most of the SubModuleCdcs as well, except there is one place we use it in the code for Versions. Is this something that should be changed or might cause an issue?

Edit: We decided internally to change the handling of Versions for other reasons

@colin-axner
Copy link
Contributor

Found some other places we use SubModuleCdc. Is this something we should try to replace by using the application defined codec?

@jackzampolin
Copy link
Member Author

Closing as addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants