komputing / KEthereum

Kotlin library for Ethereum
MIT License
350 stars 73 forks source link

add `BouncyCastleProvider()` on `Mnemonic` init #21

Closed mirceanis closed 6 years ago

mirceanis commented 6 years ago

This PR fixes #20

Normally this would not be noticeable in a project that already uses Keys from the crypto module, and it could be worked around by the user of the library by making sure to add BouncyCastleProvider before using the Mnemonic object but it should not be required.

mirceanis commented 6 years ago

I still can't assign myself to issues, nor set labels

ligi commented 6 years ago

@mirceanis can you check if you can assign yourself now - gave you write permissions - should work (tm) ..

ligi commented 6 years ago

What do you think about the following approach?:

init {
        Security.insertProviderAt(BouncyCastleProvider(), 1)
}

not a big fan of multiple calls of Security.insertProviderAt() Also perhaps this way we can make this optional or only for android as this is not needed for JVM apps consuming this lib

mirceanis commented 6 years ago

What you are saying makes a lot of sense; I'll try it

mirceanis commented 6 years ago

It seems that init {} blocks can't sit outside of classes and that classes have to be used for init blocks to be called. Either that, or I don't know my kotlin. It would have been a really elegant solution. I believe it's the same with java static {} blocks.

I looked at the documentation for insertProviderAt and it seems to be safe to call it multiple times for the same provider. It simply returns -1 if the provider already exists.

That being said, I think it's safer to use insertProviderAt directly instead of jumping through hoops to get the JVM to call init {} blocks for the optional project.

ligi commented 6 years ago

can't we just make some object around it?:

object SpongyInitializer {
 init {
        Security.insertProviderAt(BouncyCastleProvider(), 1)
 }
}
mirceanis commented 6 years ago

Of course we can, but the code that needs the provider would have to interact with that object somehow to make the init block execute and since the module can be optional, it seems we'd just be introducing hoops to jump through unnecessarily.

ligi commented 6 years ago

are you sure there is an interaction needed?

mirceanis commented 6 years ago

Yes.

The Keys object does exactly what SpongyInitializer would do.

object Keys {
...
    init {
        Security.insertProviderAt(org.spongycastle.jce.provider.BouncyCastleProvider(), 1)
    }

And it is imported by the bip39 module that has Mnemonic but the crash occurs when the Mnemonic class is used without previous interaction with Keys

ligi commented 6 years ago

Interesting - thanks! Will merge it now - but create a follow-up ticket to perhaps improve here in the future.