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

Review seqner usage (decimal vs hex) #138

Open
rodolfomiranda opened this issue Nov 16, 2023 · 2 comments
Open

Review seqner usage (decimal vs hex) #138

rodolfomiranda opened this issue Nov 16, 2023 · 2 comments
Assignees

Comments

@rodolfomiranda
Copy link
Collaborator

Review if we are using/calling the seqner correctly in a bunch of places in the code regarding sequence number being decimal or hexadecimal. It'd be helpful to add tests that run over 10 key events.

I assigned the issue to myself as a reminder.

@rodolfomiranda rodolfomiranda self-assigned this Nov 16, 2023
@lenkan
Copy link
Collaborator

lenkan commented Dec 9, 2023

@rodolfomiranda I encountered this I think. Currently we do const sn = Number(state.s) which only works for state.s < 10. If state.s is expected to be in hexadecimal without the 0x prefix, we should do this instead: const sn = parseInt(state.s, 16). I have a fix on my fork, but it contains some other fixes that I need to branch out as well. I probably won't get to it until monday.

@psteniusubi
Copy link
Contributor

Keripy definition for numbers from Number:

    Parameters:
        num (int | str | None): non-negative int number or hex str of int
            number or 0 if None
        numh (str):  string equivalent of non-negative int number

    Note: int("0xab", 16) is also valid since int recognizes 0x hex prefix

I think there's a logic error in CesrNumber of signify-ts. It does not match the logic of Number in keripy

if (typeof num == 'number') {
_num = num;
} else if (numh != undefined) {
_num = parseInt(numh, 16);
} else {
_num = 0;
}

vs

https://github.com/WebOfTrust/keripy/blob/86c50b466f3fb6b8a55615ce40ba1cc31d10b749/src/keri/core/coring.py#L1582-L1597

Also there are probably issues using parseInt with large integers (bigger than safe integer or 2^53)

CesrNumber should be fixed. Review code for uses of parseInt(input) or Number(input). Where needed replace with

const result = new CesrNumber({}, input, undefined).num

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

No branches or pull requests

3 participants