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: make gnovm wasm-friendly #1012

Merged
merged 21 commits into from
Aug 10, 2023
Merged

feat: make gnovm wasm-friendly #1012

merged 21 commits into from
Aug 10, 2023

Conversation

moul
Copy link
Member

@moul moul commented Aug 1, 2023

Fixes #1008
Addresses #1013

Contributors' checklist...
  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests
  • Added new benchmarks to generated graphs, if any. More info here.

h/t @ilgooz (pair-programming session)

Signed-off-by: Manfred Touron <94029+moul@users.noreply.github.com>
@moul moul self-assigned this Aug 1, 2023
@moul moul changed the title dev/moul/wasm feat: wasm support Aug 1, 2023
@moul moul changed the title feat: wasm support feat: wasm-friendly Aug 1, 2023
@github-actions github-actions bot added the 📦 🌐 tendermint v2 Issues or PRs tm2 related label Aug 1, 2023
Signed-off-by: Manfred Touron <94029+moul@users.noreply.github.com>
@@ -117,14 +117,6 @@ func (ndb *nodeDB) SaveNode(node *Node) {
// Has checks if a hash exists in the database.
func (ndb *nodeDB) Has(hash []byte) bool {
key := ndb.nodeKey(hash)

if ldb, ok := ndb.db.(*dbm.GoLevelDB); ok {
Copy link
Member Author

Choose a reason for hiding this comment

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

Hey @jaekwon, something seems off here. GoLevelDB.Has() is essentially checking if GoLevelDB.Get() is not nil, making this condition alter the function's behavior to panic instead of returning false, specifically when using a particular driver.

Would you be alright with removing this code, or do you have a different approach in mind?

Copy link
Contributor

Choose a reason for hiding this comment

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

PR should be ready after resolving this.

Co-authored-by: İlker G. Öztürk <ilkergoktugozturk@gmail.com>
@moul moul changed the title feat: wasm-friendly feat: make gnovm wasm-friendly Aug 1, 2023
Signed-off-by: Manfred Touron <94029+moul@users.noreply.github.com>
Signed-off-by: Manfred Touron <94029+moul@users.noreply.github.com>
Signed-off-by: Manfred Touron <94029+moul@users.noreply.github.com>
Signed-off-by: Manfred Touron <94029+moul@users.noreply.github.com>
Signed-off-by: Manfred Touron <94029+moul@users.noreply.github.com>
@moul moul mentioned this pull request Aug 1, 2023
@ilgooz
Copy link
Contributor

ilgooz commented Aug 1, 2023

Here is my counter PR based in this one.

@ilgooz
Copy link
Contributor

ilgooz commented Aug 2, 2023

@moul one more PR: moul#4

chore: fix database adapter and bump gnolang/goleveldb version
@ilgooz
Copy link
Contributor

ilgooz commented Aug 2, 2023

cc @jeronimoalbi for the failing db checks

@jeronimoalbi
Copy link
Member

cc @jeronimoalbi for the failing db checks

The issue seems to be either a new feature or bug introduced in goleveldb at some point after v1.0.0 release.
They don't have any tags after that one.

The tests break because setting []byte("") as value returns nil when Get() is called.

I think we should apply your goleveldb changes to changeset 9d007e4 which is the one that was tagged as v1.0.0. Maybe the fork should remove the changesets that followed v1.0.0.

@ilgooz
Copy link
Contributor

ilgooz commented Aug 4, 2023

@jeronimoalbi thanks for debugging, I'll make a PR.

@ilgooz
Copy link
Contributor

ilgooz commented Aug 4, 2023

Fixed in this PR: moul#5

@moul moul marked this pull request as ready for review August 7, 2023 14:52
@moul moul requested review from jaekwon and a team as code owners August 7, 2023 14:52
@zivkovicmilos zivkovicmilos self-requested a review August 7, 2023 15:02
@moul
Copy link
Member Author

moul commented Aug 10, 2023

Changes justifying a fork of goleveldb: syndtr/goleveldb@4512381

@moul
Copy link
Member Author

moul commented Aug 10, 2023

Next steps regarding DB: #1013

@moul moul merged commit 5a1d497 into gnolang:master Aug 10, 2023
77 checks passed
@moul moul deleted the dev/moul/wasm branch August 10, 2023 16:55
Doozers pushed a commit to Doozers/gno that referenced this pull request Aug 31, 2023
Co-authored-by: İlker G. Öztürk <ilkergoktugozturk@gmail.com>
Co-authored-by: İlker G. Öztürk <ilker@ilgooz.com>
@moul moul added this to the 🌟 main.gno.land (wanted) milestone Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🌐 tendermint v2 Issues or PRs tm2 related
Projects
Status: Done
Status: ✅ Done
Archived in project
Archived in project
Development

Successfully merging this pull request may close these issues.

Add support for WASM in goleveldb dependency
4 participants