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

Use multiplatform big numbers #96

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

fullkomnun
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

@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)

@ligi
Copy link
Member

ligi commented Feb 14, 2021

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

@fullkomnun
Copy link
Contributor Author

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?

@ionspin
Copy link

ionspin commented Feb 15, 2021

  • 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
Copy link

ionspin commented Feb 15, 2021

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.

3 participants