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

Remove usage of web3j library #148

Merged
merged 8 commits into from
Jun 9, 2022
Merged

Conversation

zilm13
Copy link
Contributor

@zilm13 zilm13 commented Jun 3, 2022

PR Description

Removes web3j-crypto and web3j-rlp usage across code in favor of tuweni

Design consideration:

There is a SecretKey in tuweni-crypto with Bytes32 source of the private key which is not copied until you use some guarded Bytes source, this SecretKey implements finalize filling source with 00's, and when you create another SecretKey with the same Bytes32 source and second SecretKey marked to be collected with GC, private key is erased in both as both uses the same instance of Bytes32 (it was not actually copied). From one side it's a good thing to guarantee erasure of data but it forces to use single SecretKey instance all over the library avoiding to create it when really needed.

I've considered wrapping SecretKey with some Signer interface to hide implementation and benefit from test's NoopSigner, but when parameterized it goes straight from IdentitySchemaInterpreter to NodeRecord which is used almost in every class in the package and should take more time to implement while I already spent more time than I should on this task.

Fixed Issue(s)

Fixes #147

Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. Just a few minor things and a couple of queries.

private static final int RECIPIENT_KEY_LENGTH = 16;
private static final int INITIATOR_KEY_LENGTH = 16;
private static final int AUTH_RESP_KEY_LENGTH = 16;
private static final int MS_IN_SECOND = 1000;

private static final Supplier<SecureRandom> SECURE_RANDOM = Suppliers.memoize(SecureRandom::new);

private static boolean SKIP_SIGNATURE_VERIFY = false;
static {
Security.addProvider(PROVIDER = new BouncyCastleProvider());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to init PROVIDER inline and then just have

Suggested change
Security.addProvider(PROVIDER = new BouncyCastleProvider());
Security.addProvider(PROVIDER);

That said, I don't see where PROVIDER is being used. Why did this move from Hashes to here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this move from Hashes to here?

Good question, by some (static) magic it run before everything until this update and doesn't run anymore. I've refactored it a bit to be sure it runs before anything BC-dependent

@zilm13 zilm13 requested a review from ajsutton June 7, 2022 11:49
Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Nice work.

@zilm13 zilm13 merged commit 1ffc42f into Consensys:master Jun 9, 2022
@zilm13 zilm13 deleted the replace-crypto branch June 9, 2022 08:59
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.

Replace web3j crypto
2 participants