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

refactor(stdlibs): remove Hash, GetTimestamp, FormatTimestamp from std #657

Merged
merged 3 commits into from
May 25, 2023

Conversation

thehowl
Copy link
Member

@thehowl thehowl commented Mar 27, 2023

GetTimestamp and FormatTimestamp have been deprecated a while ago, and since the language is still changing and these are used nowhere in the source except for a test, it makes sense to remove them.

std.Hash comes from a discussion in #580. rg std.Hash returns nothing in the gno source, and in a bit of an obscure way, is not just a call to Sha256Sum, but a 20-byte version of that (see pkgs/gnolang/hash_image.go). I also removed some dead code in hash_image.go.

@thehowl thehowl requested a review from a team as a code owner March 27, 2023 18:22
@thehowl thehowl self-assigned this Mar 27, 2023
Copy link
Member

@moul moul left a comment

Choose a reason for hiding this comment

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

It looks good to me, however, I’d like to ask @jaekwon if he sees good reasons to still keep this, because he was the original author.

@thehowl
Copy link
Member Author

thehowl commented Apr 5, 2023

Merged master and pinging @jaekwon to review this

@ajnavarro ajnavarro added 📦 🤖 gnovm Issues or PRs gnovm related 🌟 improvement performance improvements, refactors ... labels May 18, 2023
@jaekwon
Copy link
Contributor

jaekwon commented May 24, 2023

reading

@jaekwon
Copy link
Contributor

jaekwon commented May 24, 2023

I think this might be the right thing to do, but it's a bit weird.

One, we need to make sure that the declaration of type Location is safe, for when Location is ever set (now or in the future). So for example, Location shouldn't be modifiable, otherwise any user can modify a *Location for all users, a bad security breach. Not only should we scan the stdlibs/time code, but also I guess everything about the current implementation, including functions that haven't been ported yet, to make sure it will always be safe to completely port it and maintain it.

Also, it seems a bit unnecessary to support this time implementation because (a) it is a struct, which is more expensive than a primary value, (b) at least in the blockchain context we don't need to support that weird "wall timer" thing, (c) it honestly seems way too complex even in Go because of the "wall timer" thing. So I imagine we may want to implement another "time" package for gno in the future that is simpler.

But anyways, the stdlibs/time.Format is already used for boards, so seems fine to go ahead for now.

@thehowl
Copy link
Member Author

thehowl commented May 25, 2023

@jaekwon Agreed in many ways, though I think maybe a good idea to port these thoughts to an issue tracking this.

My thoughts balooned into a large wall of text, so I've created #851 to track this.

I'll be merging this in the meantime.

@thehowl thehowl merged commit 9726b18 into gnolang:master May 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🤖 gnovm Issues or PRs gnovm related 🌟 improvement performance improvements, refactors ...
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants