-
-
Notifications
You must be signed in to change notification settings - Fork 13
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
Tests for convertRadix
and convertRadix2
#25
Comments
convertRadix and convertRadix2 are different algorithms:
BigInts are still very slow, 5x slower for convertRadix, convertRadix2 will be even slower. You can implement convertRadix2Big if you need bigger numbers. Or convertRadix2Int, using modulo division instead of masks, it will be slower than current implementation, but will allow numbers up to 2**53 |
should be clarified in readme and code tho |
I'm completely confused the implementations Using const data = [1];
console.log(utils.radix(2 ** 5).encode(new Uint8Array(data)))
// [ 1 ]
console.log(utils.radix2(5).encode(new Uint8Array(data)))
// [ 0, 4 ] Is it the issue or desired behavior? I think implementation logic should be clarified and/or tests added. If the behavior is different maybe there is sense to rename methods? |
radix has no padding, radix2 has padding. that's main diff. |
const { convertRadix, convertRadix2, radix, radix2 } = require('./lib/index.js');
// > But I've faced with that simple test gives incorrect result:
// No, this is actually correct result, 1 % 2**8 === 1 % 2**5
console.log(convertRadix([1], 2 ** 8, 2 ** 5)); // -> [ 1 ]
// If it element is bigger or equal than 2**5, it will overflow to next element
console.log(convertRadix([2 ** 5], 2 ** 8, 2 ** 5)); // [ 1, 0 ]
// Same happens with convertRadix2:
// NOTE: since u8 = 2 u4, it will always return two elements for single one
console.log(convertRadix2([1], 8, 4, false)); // [ 0, 1 ]
console.log(convertRadix([1], 2 ** 8, 2 ** 4)); // -> [ 1 ] (can be represented as 1 elm)
// If bigger -> will overflow to next element
console.log(convertRadix2([2 ** 4], 8, 4, false)); // [ 1, 0 ]
console.log(convertRadix([2 ** 4], 2 ** 8, 2 ** 4)); // -> [ 1, 0 ] (same!)
// What happens with 2**8 -> 2**5?
// Since we cannot represent 1 8bit number as multiple 5bit number, we need to add padding
console.log(convertRadix2([0, 0, 0, 0, 1], 8, 5)); // [ 0, 0, 0, 0, 0, 0, 0, 1 ]
// Otherwise there will be error:
console.log(convertRadix2([1], 8, 5, true)); // [ 0, 4 ]
// Reverse. NOTE: no padding here, because we add it on encoding
console.log(convertRadix2([0, 4], 5, 8)); // [ 1 ] |
I understand. But it is really hard to understand why this is correct. I think at least a mention in the readme is necessary. |
I've used
convertRadix
function on the cashaddr implementation #24 and it converts 5-bit number correctly according to the checksum tests.But I've faced with that simple test gives incorrect result:
I thought that I could understand the behavior from tests, but there are no tests for these methods.
Is this my mistake in use or is the method not implemented correctly?
The text was updated successfully, but these errors were encountered: