KEthereum icon indicating copy to clipboard operation
KEthereum copied to clipboard

Use multiplatform big numbers

Open fullkomnun opened this issue 3 years ago • 6 comments

Part of an overall effort to make KEthereum multiplatform (focusing on JVM/JS). Trying to replace java.math.BigInteger and java.math.BigDecimal with multiplatform alternatives from 'kotlin-multiplatform-bignum' as already done successfully as part of multiplatform support for KHash.

Started by adding the jvm variant as a common dependency to all sub-modules followed by converting both main and test sources. Fixed some issues but it is still WIP and pending some issues reported that result in test failures.

The only usages of java.math.BigInteger are in crypto jvm-specific implementation such as SpongyCastle or BouncyCastle that internally use java's BigInteger to interact with jvm libraries. The crypto API uses kotlin BigInteger and underlying implementation can convert from/to java's.

fullkomnun avatar Feb 09 '21 10:02 fullkomnun

The bugs in 'kotlin-multiplatform-bignum' have been resolved. I made a small amendment to RLPTypeConverter.kt to remove leading zero bytes since the result of kotlin BigInteger's BigInteger::toByteArray has some additional leading zeros. The only thing missing for the conversion is the update Kethabi in parallel (which I tested locally and than all tests pass).

fullkomnun avatar Feb 12 '21 05:02 fullkomnun

@ligi Conversion is almost complete, would love to get your feedback!

Some Notes about migration:

  • kotlin-multiplatform-bignum supports all multiplatform targets but we currently use the jvm-target as an intermediate phase.
  • BigInteger(1, bytes) => BigInteger.fromByteArray(bytes, Sign.POSITIVE)
  • BigInteger("1") => BigInteger.parseString("1") [default base is '10']
  • BigInteger(hexString, 16) => BigInteger.parseString(hexString, 16)
  • big.shl(1) => big shl 1 [same for shr]
  • BigInteger.valueOf(37) => BigInteger(37) [same for other primitives]
  • big.toLong() => big.longValue() [same for other primitives]
  • big.intValueExact() => big.intValue(exactRequired = true)
  • BigInteger(bytes) => BigInteger.fromTwosComplementByteArray(bytes)
  • big.toByteArray() => big.toTwosComplementByteArray()
  • RLPTypeConverter.kt : when using BigInteger::toByteArray kotlin's BigInteger has extra leading zero bytes that had to be discarded by util method from 'extensions_kotlin' module
  • Kotlin's JVM extensions for primitives use Java's BigInteger and should not be used (1L.toBigInteger() => BigInteger(1L))
  • The only modules that still use java.math.BigInteger internally are crypto jvm implementations such as bouncy castle and spongy castle but crypto common types and API's all use 'kotlin-multiplatform-bignum'.
  • 'BigDecimal' is used rarely(erc67 and erc681) and was also migrated to multiplatform
  • Since Kethabi and KEthereum depend on each other and both have to be updated, it makes things a bit harder...
  • bignum was added as a dependency to all sub-modules for simplicity during migration (will try to change this to a more granular config by usage)

fullkomnun avatar Feb 14 '21 04:02 fullkomnun

In general it looks like really good work! I would just wait with merging until:

  • we can use a stable release of the ionspin library (hoping this is not too far off) I would love to not use snapshots
  • the change to the kethabi is upstreamed (the isChanging = true and resolutionstrategy is a real deal breaker to me) - and yea - I also do not like this weird coupling of kethabi and kethereum - but I really have no better idea how to do it ..
  • did you ask the ionspin regarding the extra leading zeroes ? Just removing them in the util sounds like a weird workaround to me - wondering if it is intended behavior from the lib or a bug

ligi avatar Feb 14 '21 05:02 ligi

In general it looks like really good work! I would just wait with merging until:

  • we can use a stable release of the ionspin library (hoping this is not too far off) I would love to not use snapshots
  • the change to the kethabi is upstreamed (the isChanging = true and resolutionstrategy is a real deal breaker to me) - and yea - I also do not like this weird coupling of kethabi and kethereum - but I really have no better idea how to do it ..
  • did you ask the ionspin regarding the extra leading zeroes ? Just removing them in the util sounds like a weird workaround to me - wondering if it is intended behavior from the lib or a bug

I agree. Regarding the leading zeros I think I fixed the bug and submitted a PR. We will wait for the next stable release of 'kotlin-multiplatform-bignum'.

Can you please look at this PR?

fullkomnun avatar Feb 15 '21 07:02 fullkomnun

  • we can use a stable release of the ionspin library (hoping this is not too far off) I would love to not use snapshots

Leading zeroes fix is merged and I'll try to get a stable release today or tomorrow.

ionspin avatar Feb 15 '21 08:02 ionspin

0.2.8 is released https://repo1.maven.org/maven2/com/ionspin/kotlin/bignum/

ionspin avatar Feb 15 '21 10:02 ionspin