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

feat(stdlibs): import crypto/sha256 #580

Merged
merged 1 commit into from
Mar 27, 2023
Merged

feat(stdlibs): import crypto/sha256 #580

merged 1 commit into from
Mar 27, 2023

Conversation

albttx
Copy link
Member

@albttx albttx commented Mar 9, 2023

All credit to @r3v4s and his PR #570

Description

Import function Sum256 from package crypto/sha256 .

My goal is to implement a merkle-airdrop contract, so i need Sum256 function.

How has this been tested?

see unit test TestSha256Sum

@albttx albttx requested a review from a team as a code owner March 9, 2023 12:50
@moul moul mentioned this pull request Mar 10, 2023
@r3v4s
Copy link
Contributor

r3v4s commented Mar 11, 2023

Hello @albttx, thx for you comment back on #570.

I have question for this pr.
For me it looks Jae already implemented sha256 in gno.

https://github.com/gnolang/gno/blob/master/stdlibs/stdlibs.go#L159
https://github.com/gnolang/gno/blob/master/pkgs/gnolang/hash_image.go

What do you think??

@albttx
Copy link
Member Author

albttx commented Mar 11, 2023

@r3v4s you're right, but i believe Gno should match as much as possible go stdlib and so set this function under crypto/sha256.

Maybe a following PR could be to remove this std.Hash

cc: @moul ?

@moul
Copy link
Member

moul commented Mar 11, 2023

In general, let's try being as close as possible; but I'm sure many good reasons will show that we could be different.

Right now I suggest keeping both, the idiomatic way and std.Hash. I don't have enough feedback or feeling yet.

@moul moul requested a review from jaekwon March 12, 2023 16:00
@moul
Copy link
Member

moul commented Mar 13, 2023

@jaekwon could you look at this and #589, please?

@albttx
Copy link
Member Author

albttx commented Mar 13, 2023

And what do you think about adding at the same time sha256.New ?

@jaekwon
Copy link
Contributor

jaekwon commented Mar 21, 2023

sha256.New is difficult... in stdlibs we want to limit the things to things that can be persisted, and native objects like Go's sha256.New cannot be persisted.

So we would have to write a Gno sha256.Digest shim.
Even with the shim, Go's sha256.digest is not exposed, so we would need to port it to pkgs/crypto/sha256exposed (say).
With the shim and state transfer per operation it could work easily assuming it is easy to copy the .digest in and out, and compared to hash operations this should be fairly quick, so I think this is doable.

But it seems like a lot compared to just exposing some helper methods without .New(), for now.

@jaekwon
Copy link
Contributor

jaekwon commented Mar 21, 2023

This PR as is with Sha256() seems good though!

@albttx
Copy link
Member Author

albttx commented Mar 21, 2023

rebased and fixed lint

@albttx albttx closed this by deleting the head repository Mar 21, 2023
@albttx albttx reopened this Mar 21, 2023
@albttx
Copy link
Member Author

albttx commented Mar 27, 2023

@gnolang/core-tech can this be merge ?

Copy link
Member

@thehowl thehowl left a comment

Choose a reason for hiding this comment

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

Merging as this is also pretty much the same code-wise as std.Hash... I did notice, however, that std.Hash and sha256.Sum256 are not the same, because std.Hash trims the result to 20 bytes (from 32).

Judging from the references it is mostly used a few times internally, but probably doesn't have a real purpose in real gno code. It may make sense to remove it. I'll make an issue to discuss that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

5 participants