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): add encoding, encoding/{base32,binary,csv} #1290

Open
wants to merge 41 commits into
base: master
Choose a base branch
from

Conversation

notJoon
Copy link
Member

@notJoon notJoon commented Oct 25, 2023

WIP

  • encoding
  • encoding/base32
  • encoding/binary
  • encoding/csv : Due to the reflect does not implemented yet, skipped fuzz

  1. encoding/asn1: better after reflection

related to:

depends-on:

@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Oct 25, 2023
@codecov
Copy link

codecov bot commented Oct 25, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

@notJoon notJoon marked this pull request as ready for review October 26, 2023 08:28
@notJoon notJoon requested review from jaekwon, moul and a team as code owners October 26, 2023 08:28
@thehowl thehowl marked this pull request as draft October 26, 2023 16:49
@thehowl
Copy link
Member

thehowl commented Oct 26, 2023

converting to draft for now, mark as ready when it's done :)

full implementation of encoding/binary requires reflection; not sure what category I put it into but if you just do varint it should be good 👍

@notJoon
Copy link
Member Author

notJoon commented Oct 27, 2023

@thehowl Thanks! I'll trying to finishing varint first 👍 👍

@notJoon
Copy link
Member Author

notJoon commented Oct 27, 2023

test
The original test code encountered a segment fault. So, I converted the exmaple code in the package document to gno test code.

@moul
Copy link
Member

moul commented Oct 29, 2023

don't forget to update the docs/ folder, thank you.

@notJoon
Copy link
Member Author

notJoon commented Oct 30, 2023

@moul don't worry, I'll update together when it's done.

@notJoon
Copy link
Member Author

notJoon commented Oct 31, 2023

base32 testing result

Finally solved the base32 decoder part that wasn't working. Additionaly, I also added some use cases to validate the package's functionalities.

@notJoon notJoon marked this pull request as ready for review October 31, 2023 02:42
@thehowl thehowl changed the title feat: add encoding stdlib feat(stdlibs)): add encoding, encoding/{base32,binary,csv} Nov 9, 2023
@thehowl thehowl changed the title feat(stdlibs)): add encoding, encoding/{base32,binary,csv} feat(stdlibs): add encoding, encoding/{base32,binary,csv} Nov 9, 2023
@thehowl
Copy link
Member

thehowl commented Nov 9, 2023

@notJoon please fix tests :) (make _test.gnolang.stdlibs in gnovm, and run make fmt as well)

if you need any help ping me on signal

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.

I made a bunch of gymnastics to try as much as possible to copy over tests and code from the Go stdlib, with some decent amount of success.

I put some fixes in to base32, made varint be 1:1 with the go stdlibs. I also worked a bunch on the csv package, but for now I'm giving up on continuing the fixes.

You're welcome to pick up where I left off. (If you don't, I'll probably pick it up some other day.) Use the following command from gnovm to do tests:

go test tests/*.go -run "TestPackages/(encoding)/csv" -v

I'll add this command to the meta stdlibs issue.

Thank you for your efforts!

gnovm/stdlibs/encoding/base32/base32_test.gno Outdated Show resolved Hide resolved
gnovm/stdlibs/encoding/base32/base32.gno Outdated Show resolved Hide resolved
gnovm/stdlibs/encoding/binary/varint.gno Outdated Show resolved Hide resolved
gnovm/stdlibs/encoding/binary/varint_test.gno Show resolved Hide resolved
thehowl added a commit that referenced this pull request Nov 16, 2023
A very simple ID generation package, designed to be used in combination
with `avl.Tree`s to push values in order.

The name was originally `seqid` (sequential IDs), but after saying it a
few times I realised it was close to "squid" and probably would be more
fun if I named it that way ;)

There's another piece of functionality that I want to add, which is a
way to create simple base32-encoded IDs. This depends on #1290. These
would also guarantee alphabetical ordering, so a list of them can be
easily sorted and you'd get it in the same order they were created. They
would likely be 13 characters long, but I'm also thinking of making a
compact version which works from [0,2^35) which is 7 chracters, and then
smoothly transitions over to the 13 characters version when the ID is
reached.

(I've experience with both base64 and base32 encoded IDs as 64-bit
numbers, as I used both systems. The advantage of base32 is that it
makes IDs case insensitive, all the while being at most 13 bytes instead
of 11 for base64.)

In GnoChess, we used simple sequential IDs combined with
[`zeroPad9`](https://github.com/gnolang/gnochess/blob/7e841191a4a0a94c0d46bc977458bd6b757eab5e/realm/chess.gno#L287-L296)
to create IDs which were both readable and sortable. I want to make a
more "canonical" solution to this which does not have a upper limit at 1
billion entries.
@thehowl thehowl self-assigned this Dec 22, 2023
@zivkovicmilos zivkovicmilos requested a review from thehowl March 29, 2024 03:16
@zivkovicmilos
Copy link
Member

@thehowl What's the status on this PR?

@thehowl
Copy link
Member

thehowl commented Apr 9, 2024

@zivkovicmilos current status: some tests still failing, need to investigate and finish them up; putting this back in draft for now

@thehowl thehowl marked this pull request as draft April 9, 2024 15:23
@Gno2D2
Copy link
Collaborator

Gno2D2 commented Dec 8, 2024

🛠 PR Checks Summary

All Automated Checks passed. ✅

Manual Checks (for Reviewers):
  • SKIP: Do not block the CI for this PR
Read More

🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers.

✅ Automated Checks (for Contributors):

🟢 Maintainers must be able to edit this pull request (more info)
🟢 The pull request head branch must be up-to-date with its base (more info)

☑️ Contributor Actions:
  1. Fix any issues flagged by automated checks.
  2. Follow the Contributor Checklist to ensure your PR is ready for review.
    • Add new tests, or document why they are unnecessary.
    • Provide clear examples/screenshots, if necessary.
    • Update documentation, if required.
    • Ensure no breaking changes, or include BREAKING CHANGE notes.
    • Link related issues/PRs, where applicable.
☑️ Reviewer Actions:
  1. Complete manual checks for the PR, including the guidelines and additional checks if applicable.
📚 Resources:
Debug
Automated Checks
Maintainers must be able to edit this pull request (more info)

If

🟢 Condition met
└── 🟢 The pull request was created from a fork (head branch repo: notJoon/gno-core)

Then

🟢 Requirement satisfied
└── 🟢 Maintainer can modify this pull request

The pull request head branch must be up-to-date with its base (more info)

If

🟢 Condition met
└── 🟢 On every pull request

Then

🟢 Requirement satisfied
└── 🟢 Head branch (notJoon:add-encoding) is up to date with base (master): behind by 0 / ahead by 41

Manual Checks
**SKIP**: Do not block the CI for this PR

If

🟢 Condition met
└── 🟢 On every pull request

Can be checked by

  • Any user with comment edit permission

@thehowl thehowl marked this pull request as ready for review December 8, 2024 12:18
@thehowl
Copy link
Member

thehowl commented Dec 8, 2024

PR is now ready, but depends on two other PRs to fix VM bugs which were causing problems in the tests

@Kouteki Kouteki added the in focus Core team is prioritizing this work label Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in focus Core team is prioritizing this work 📦 🤖 gnovm Issues or PRs gnovm related
Projects
Status: Backlog
Status: In Review
Development

Successfully merging this pull request may close these issues.

6 participants