Skip to content

KAMT: Remove or change mapping of integers to keys #2123

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

Open
Stebalien opened this issue Apr 4, 2025 · 2 comments
Open

KAMT: Remove or change mapping of integers to keys #2123

Stebalien opened this issue Apr 4, 2025 · 2 comments

Comments

@Stebalien
Copy link
Member

Right now we map integers to KAMT keys by encoding them with "native endian":

  1. This is non-portable, but almost always going to be little-endian.
  2. Little endian is never what we actually want. We want keys that are close to have prefixes that are close, that's how the KAMT works.

Fortunately, nobody (that I know of) is actually using this feature. We _always use byre-array keys, not integers (except in tests). So, our options here are:

  1. Switch from "native endian" to "little endian". This is the "least breaking" fix, but we don't want little endian.
  2. Switch from "native endian" to "big endian". This is the "silently breaking" fix, but it does what we want.
  3. Remove built-in support for integer keys. This is the "safely breaking" fix. This is the fix we should go with as good OSS stewards but... the real question here is, does anybody else actually care?
@github-project-automation github-project-automation bot moved this to 📌 Triage in FilOz Apr 4, 2025
@rvagg
Copy link
Member

rvagg commented Apr 4, 2025

When I went trying to test the KAMT on live data I was surprised to find that I had to import a ton of junk over from builtin-actors where it defines its own key type. Is a U256 key type not a good thing to include in a shared library somewhere and also make it available for testing purposes?
I like the idea of having a simple integer type for basic testing, maybe that code shouldn't be exported though. But I'd also prefer the tests in here go a bit further and dabble in realistic data.

@Stebalien
Copy link
Member Author

So, keys are implemented for [u8; 32] (and 20 and 64 bytes as well). Unfortunately, we can't implement the key traits for integer types without exporting those implementations, so we need to make a decision.

I agree that implementing the traits for integers is something we really should be doing.

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

No branches or pull requests

2 participants