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

Add SHA-512/256 hash op and FIXED32_LITTLE length op #39

Merged
merged 2 commits into from
Apr 19, 2021

Conversation

kostko
Copy link
Contributor

@kostko kostko commented Apr 19, 2021

See discussion in cosmos/ibc#554.

Copy link
Contributor

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Thanks for this PR and nice to point out https://github.com/confio/ics23/pull/39/files#diff-946b8d2ee64b4b178e19710fbd2c831353ccef71968db74f051f482ca747afb4R167-R171

We aim to keep all 3 language implementations in sync, so this needs to be implemented for rust and typescript (in other folders).

If you can implement them, that would be great. If not, I will put this on my TODO list.

@kostko
Copy link
Contributor Author

kostko commented Apr 19, 2021

Whoops, missed that, yeah I can add the other implementations.

@kostko kostko force-pushed the kostko/feature/more-ops branch 2 times, most recently from f3c90cd to 988ab08 Compare April 19, 2021 13:07
@kostko
Copy link
Contributor Author

kostko commented Apr 19, 2021

@ethanfrey Added the rest, Javascript is the most annoying.

Copy link
Contributor

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Looks good. One minor comment.

If you do fix that, please run cargo fmt on the rust code as well. Then it is good to merge.

js/src/ops.ts Outdated
let l = n;
for (let i = enc.length; i > 0; i--) {
/* tslint:disable */
enc[Math.abs(i - enc.length)] = l;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would do = (l % 256). That is quite clear what you want. I think this will happen automatically in JS, but it is harder to review and ensure correctness.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah makes sense, I prefer bitwise ops (e.g., l & 0xFF), but the linter doesn't like them for some reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

@kostko kostko force-pushed the kostko/feature/more-ops branch from 988ab08 to 346d8d9 Compare April 19, 2021 18:18
Copy link
Contributor

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for your contribution.

@ethanfrey ethanfrey merged commit 267cfba into cosmos:master Apr 19, 2021
@ethanfrey
Copy link
Contributor

ethanfrey commented Apr 19, 2021

I pushed 0.6.5 rust crate and npm package.

I tagged for go, but I got some errors trying to update local files. I will try again for that.

There was a solution in #35 but I just get errors.

@kostko
Copy link
Contributor Author

kostko commented Apr 20, 2021

I tagged for go, but I got some errors trying to update local files. I will try again for that.

There was a solution in #35 but I just get errors.

Seems to work for me with both v0.6.5 and v0.6.6 (since your module is in a subfolder you need to use e.g. go/v0.6.6 as tags -- which you are). Note that you need to import this as github.com/confio/ics23/go so not sure why you also have a top-level go.mod file.

I would just remove that one (e.g., https://github.com/confio/ics23/blob/master/go.mod and corresponding go.sum) as it is confusing and unused.

@ethanfrey
Copy link
Contributor

Note that you need to import this as github.com/confio/ics23/go so not sure why you also have a top-level go.mod file.

I would just remove that one (e.g., https://github.com/confio/ics23/blob/master/go.mod and corresponding go.sum) as it is confusing and unused.

Yes, it is confusing, and Zaki helped with the Go tagging. I can gladly remove the top level go.mod. For some reason I thought it was needed.

@ethanfrey
Copy link
Contributor

BTW, if you need this in a build of the cosmos sdk, you need to manually update the dependency. And if you want it on the hub, maybe make a PR to bump the ics23 version, so it will be included in the next release

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

Successfully merging this pull request may close these issues.

2 participants