Skip to content

Conversation

@f0i
Copy link
Contributor

@f0i f0i commented Jul 30, 2025

  • Add functions for LEB128 encoding and decoding of Nat and Int values.
  • Add doc comments to LEB128 functions
  • Add additional keywords to mops.toml

@tomijaga tomijaga requested a review from Copilot July 30, 2025 11:22
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds LEB128 encoding and decoding functions for Nat and Int types, extending the existing functionality that only supported Nat64 and Int64. It also improves documentation by adding doc comments to all LEB128 functions and updates package metadata with additional keywords.

  • Implements new LEB128 functions for Nat and Int types (both encoding and decoding)
  • Adds comprehensive documentation comments to all LEB128 functions
  • Updates package keywords to better reflect the library's functionality

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
src/lib.mo Adds new LEB128 functions for Nat/Int types and doc comments for all LEB128 functions
tests/ByteUtils.Test.mo Adds test coverage for the new Nat and Int LEB128 functions
tests/Sorted.Test.mo Removes unused imports to clean up test file
mops.toml Updates keywords array with more descriptive encoding-related terms

@NatLabs NatLabs deleted a comment from Copilot AI Jul 30, 2025
@tomijaga
Copy link
Member

Hey @f0i , thanks for the CR.
The implementation looks good, I just had one comment before I merge.

Could you add a few more test cases for Nat values greater than 2^64?
You could either add a few hard coded values or use the fuzz library to generate some.

@tomijaga
Copy link
Member

Benchmark workflows are failing for an unrelated reason - mops uses the latest moc version which can be incompatible with the base package at times. I plan to update the workflow to use the version specified in the mops.toml file

@f0i
Copy link
Contributor Author

f0i commented Jul 31, 2025

I added some test cases.
Here is the python script I used to generate the expected byte arrays without relying on my own implementation:

#!/usr/bin/env python3

import leb128

def testULEB128(name, value):
    print(name, value, [f'0x{byte:02x}' for byte in leb128.i.encode(value)])

def testSLEB128(name, value):
    print(name, value, [f'0x{byte:02x}' for byte in leb128.i.encode(value)])

print("ULEB128")
testULEB128("2 ** 64", 2 ** 64)
testULEB128("2 ** 65", 2 ** 65)
testULEB128("2 ** 70", 2 ** 70)
testULEB128("2 ** 64 + 1", 2 ** 64 + 1)
testULEB128("123456789012345678901234567890", 123456789012345678901234567890)
print("")
print("SLEB128")
testULEB128("2 ** 64", 2 ** 64)
testULEB128("2 ** 65", 2 ** 65)
testULEB128("2 ** 70", 2 ** 70)
testULEB128("2 ** 64 + 1", 2 ** 64 + 1)
testULEB128("123456789012345678901234567890", 123456789012345678901234567890)
testULEB128("-1 * (2 ** 64)", -1*(2 ** 64))
testULEB128("-1 * (2 ** 65)", -1*(2 ** 65))
testULEB128("-1 * (2 ** 70)", -1*(2 ** 70))
testULEB128("-1 * (2 ** 64 + 1)", -1*(2 ** 64 + 1))
testULEB128("-123456789012345678901234567890", -123456789012345678901234567890)

Output of that script:

ULEB128
2 ** 64 18446744073709551616 ['0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x02']
2 ** 65 36893488147419103232 ['0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x04']
2 ** 70 1180591620717411303424 ['0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x01']
2 ** 64 + 1 18446744073709551617 ['0x81', '0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x02']
123456789012345678901234567890 123456789012345678901234567890 ['0xd2', '0x95', '0xfc', '0xf1', '0xe4', '0x9d', '0xf8', '0xb9', '0xc3', '0xed', '0xbf', '0xc8', '0xee', '0x31']

SLEB128
2 ** 64 18446744073709551616 ['0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x02']
2 ** 65 36893488147419103232 ['0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x04']
2 ** 70 1180591620717411303424 ['0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x01']
2 ** 64 + 1 18446744073709551617 ['0x81', '0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x02']
123456789012345678901234567890 123456789012345678901234567890 ['0xd2', '0x95', '0xfc', '0xf1', '0xe4', '0x9d', '0xf8', '0xb9', '0xc3', '0xed', '0xbf', '0xc8', '0xee', '0x31']
-1 * (2 ** 64) -18446744073709551616 ['0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x7e']
-1 * (2 ** 65) -36893488147419103232 ['0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x7c']
-1 * (2 ** 70) -1180591620717411303424 ['0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x80', '0x7f']
-1 * (2 ** 64 + 1) -18446744073709551617 ['0xff', '0xff', '0xff', '0xff', '0xff', '0xff', '0xff', '0xff', '0xff', '0x7d']
-123456789012345678901234567890 -123456789012345678901234567890 ['0xae', '0xea', '0x83', '0x8e', '0x9b', '0xe2', '0x87', '0xc6', '0xbc', '0x92', '0xc0', '0xb7', '0x91', '0x4e']

@tomijaga
Copy link
Member

tomijaga commented Aug 1, 2025

Thanks for adding these test cases. I'll merge and update the mops version

@tomijaga tomijaga merged commit dd58c06 into NatLabs:main Aug 1, 2025
1 of 2 checks passed
@tomijaga
Copy link
Member

tomijaga commented Aug 1, 2025

@f0i v0.1.0 is published on mops: https://mops.one/byte-utils@0.1.0

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