Skip to content

Conversation

@gupnik
Copy link
Collaborator

@gupnik gupnik commented May 22, 2025

Description

This PR removes trezor crypto and migrates remaining API calls to Rust via FFI.

How to test

Run tests

Types of changes

Refactor

Checklist

  • Create pull request as draft initially, unless its complete.
  • Add tests to cover changes as needed.
  • Update documentation as needed.
  • If there is a related Issue, mention it in the description.

If you're adding a new blockchain

  • I have read the guidelines for adding a new blockchain.

gupnik added 23 commits May 13, 2025 12:08
…emove-trezor-crypto

# Conflicts:
#	rust/tw_crypto/src/crypto_hd_node/ecdsa/nist256p1/mod.rs
#	rust/tw_crypto/src/crypto_hd_node/ecdsa/secp256k1/mod.rs
#	rust/tw_crypto/src/crypto_hd_node/ed25519/cardano/mod.rs
#	rust/tw_crypto/src/crypto_hd_node/extended_key/bip32_private_key.rs
#	rust/tw_crypto/src/crypto_hd_node/extended_key/extended_private_key.rs
#	rust/tw_crypto/src/crypto_hd_node/extended_key/extended_public_key.rs
#	rust/tw_crypto/src/crypto_hd_node/extended_key/hd_version.rs
#	rust/tw_crypto/src/crypto_hd_node/hd_node.rs
#	rust/tw_crypto/src/crypto_hd_node/zilliqa_schnorr/mod.rs
#	rust/tw_crypto/src/ffi/crypto_hd_node.rs
#	rust/tw_crypto/src/ffi/crypto_hd_node_public.rs
#	rust/tw_crypto/tests/extended_private_key.rs
#	rust/tw_crypto/tests/hd_node_ffi.rs
#	rust/tw_crypto/tests/hd_node_public_ffi.rs
#	rust/tw_hash/src/hasher.rs
#	rust/tw_keypair/src/ecdsa/nist256p1/private.rs
#	rust/tw_keypair/src/ecdsa/nist256p1/public.rs
#	rust/tw_keypair/src/ecdsa/secp256k1/private.rs
#	rust/tw_keypair/src/ecdsa/secp256k1/public.rs
#	rust/tw_keypair/src/ed25519/modifications/cardano/extended_private.rs
#	rust/tw_keypair/src/ed25519/modifications/waves/private.rs
#	rust/tw_keypair/src/ed25519/private.rs
#	rust/tw_keypair/src/traits.rs
#	rust/tw_keypair/src/zilliqa_schnorr/private.rs
#	rust/tw_keypair/src/zilliqa_schnorr/public.rs
#	src/HDWallet.cpp
@gupnik gupnik requested a review from satoshiotomakan as a code owner May 22, 2025 09:04
@github-actions
Copy link

github-actions bot commented May 22, 2025

Binary size comparison

➡️ aarch64-apple-ios:

- 14.98 MB
+ 14.99 MB 	 +9 KB

➡️ aarch64-apple-ios-sim:

- 14.98 MB
+ 14.99 MB 	 +9 KB

➡️ aarch64-linux-android:

- 20.04 MB
+ 20.06 MB 	 +15 KB

➡️ armv7-linux-androideabi:

- 16.68 MB
+ 16.69 MB 	 +9 KB

➡️ wasm32-unknown-emscripten:

- 14.34 MB
+ 14.35 MB 	 +9 KB

@gupnik gupnik mentioned this pull request May 23, 2025
8 tasks
Copy link
Collaborator

@satoshiotomakan satoshiotomakan left a comment

Choose a reason for hiding this comment

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

Good job! Let's make a couple of minor improvements before merging the PR!


#[tw_ffi(ty = static_function, class = TWCurve25519, name = PubkeyToEd25519)]
#[no_mangle]
pub unsafe extern "C" fn tw_curve25519_pubkey_to_ed25519(
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like an internal function. For the future, let's add them manually, or extend tw_ffi to not expose such an FFI to Kotlin, Swift etc

/// \param is_secp256k1 whether the signature is a secp256k1 signature.
///
/// \return DER-encoded signature.
#[tw_ffi(ty = static_function, class = TWECDSA, name = SigToDER)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

TWECDSA looks like an internal as well

Copy link
Collaborator

@satoshiotomakan satoshiotomakan left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

@satoshiotomakan satoshiotomakan merged commit ee9430d into dev May 23, 2025
14 checks passed
@satoshiotomakan satoshiotomakan deleted the gupnik/remove-trezor-crypto branch May 23, 2025 07:41
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