feat: algokit_utils (Rust impl only) #166
Conversation
Had to work around this bug in maturin: PyO3/maturin#2459 This fix is relatively simple but requires you to modify the packages post-installation. A fix (more like feature) can be added to maturin and I think it should be fairly straightforward
There is something weird going on with the env package though. For now you need to bun rm env, run the build script, and then bun i env before running the tests
Mutex works in WASM and only adds a few nanos of overhead, so just easier to use Mutex for both uniffi and wasm
It works, but there seems to be some closure that isn't getting cleaned up properly in the async call
Bun prefers main over module for some reason at runtime. See oven-sh/bun#13430 (comment)
There was a problem hiding this comment.
Pull Request Overview
This PR delivers the standalone Rust implementation of algokit_utils and related FFI-supporting crates, removes the interim FFI utils packaging, and updates workspace and build configurations accordingly.
- Introduces
ffi_mutexcrate for unified mutex API across Tokio and WASM. - Adds
algokit_utilscrate and updates workspace member listings. - Adjusts
build_pkgsscript and FFI crate configurations.
Reviewed Changes
Copilot reviewed 10 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/build_pkgs/src/python.rs | Removes unused platform-specific ext logic. |
| crates/ffi_mutex/src/lib.rs | Adds FfiMutex wrapper with Tokio and WASM variants. |
| crates/ffi_mutex/Cargo.toml | Defines optional features and dependencies. |
| crates/algokit_utils/Cargo.toml | Bootstraps the new algokit_utils crate. |
| crates/algokit_transact_ffi/Cargo.toml | Updates crate-type entries to include lib. |
| crates/algokit_http_client_trait/src/lib.rs | Implements HTTP client trait for native and WASM. |
| crates/algokit_http_client_trait/Cargo.toml | Sets up features and dependencies for HTTP trait. |
| crates/algod_api/src/lib.rs | Uses the HTTP trait to implement transaction_params. |
| crates/algod_api/Cargo.toml | Configures default HTTP client feature. |
| Cargo.toml | Updates workspace member list. |
Comments suppressed due to low confidence (1)
crates/ffi_mutex/src/lib.rs:15
- Add unit tests for
FfiMutexto cover bothffi_uniffiandffi_wasmfeature branches, ensuring both locking methods behave as expected.
pub struct FfiMutex<T>(InnerMutex<T>);
| /// WASM doesn't need to be thread-safe, but we still need the interior mutability so we can have the same API on our FfiMutex in both | ||
| /// cases (otherwise we have a bunch of different code paths in our implementations). We probably could use some unsafe code | ||
| /// (i.e `UnsafeCell`) in place of the `RefCell`, but to make our lives easier and avoid accidental UB we use `RefCell` instead. The | ||
| /// perofmrance difference is likely negligible in practice. |
There was a problem hiding this comment.
There's a typo in this doc comment: 'perofmrance' should be 'performance'.
| /// perofmrance difference is likely negligible in practice. | |
| /// performance difference is likely negligible in practice. |
Copilot uses AI. Check for mistakes.
| #[async_trait(?Send)] | ||
| impl HttpClient for WasmHttpClient { | ||
| async fn get(&self, path: String) -> Result<Vec<u8>, HttpError> { | ||
| let result = self.get(&path).await.unwrap(); |
There was a problem hiding this comment.
Calling unwrap on the result of an external JS API can cause panics; consider propagating the JsValue error with map_err instead of unwrap to avoid runtime panics.
| let result = self.get(&path).await.unwrap(); | |
| let result = self.get(&path) | |
| .await | |
| .map_err(|e| HttpError::HttpError(format!("JavaScript error: {:?}", e)))?; |
Copilot uses AI. Check for mistakes.
| } | ||
|
|
||
| #[cfg(feature = "ffi_wasm")] | ||
| pub async fn lock(&self) -> std::cell::RefMut<'_, T> { |
There was a problem hiding this comment.
[nitpick] The lock method in the WASM branch is marked async but runs synchronously; consider removing async to simplify the API and avoid unnecessary Future overhead.
| pub async fn lock(&self) -> std::cell::RefMut<'_, T> { | |
| pub fn lock(&self) -> std::cell::RefMut<'_, T> { |
Copilot uses AI. Check for mistakes.
| pub min_fee: u64, | ||
| } | ||
|
|
||
| impl AlgodClient { |
There was a problem hiding this comment.
Probably worth putting some doc comments around this impl.
| @@ -0,0 +1,19 @@ | |||
| [package] | |||
| name = "algokit_http_client_trait" | |||
There was a problem hiding this comment.
Potentially we could call this algokit_http_client given that it optionally has a default implementation.
| use async_trait::async_trait; | ||
|
|
||
| #[derive(Debug, Default, Clone)] | ||
| pub struct CommonParams { |
There was a problem hiding this comment.
We call these common fields TransactionHeader in algokit_transact.
I'm wondering if we align terminology?
| if idx < txns.len() { | ||
| SignedTransaction { | ||
| transaction: txns[idx].clone(), | ||
| signature: Some([0; 64]), |
There was a problem hiding this comment.
Question: Is it better to this or use None? If we always expect a signature, then potentially this field shouldn't be optional?
| SignedTransaction { | ||
| transaction: txns[idx].clone(), | ||
| signature: Some([0; 64]), | ||
| auth_address: None, |
There was a problem hiding this comment.
What the plan for supporting rekeyed accounts?
| self.signer_getter.get_signer(address).await | ||
| } | ||
|
|
||
| // TODO: Use Fn defined in ComposerConfig |
There was a problem hiding this comment.
Is this comment about adding support for caching suggested params?
| .transactions | ||
| .iter() | ||
| .map(|composer_txn| { | ||
| let alread_formed_txn = matches!(composer_txn, ComposerTxn::Transaction(_)); |
There was a problem hiding this comment.
| let alread_formed_txn = matches!(composer_txn, ComposerTxn::Transaction(_)); | |
| let already_formed_txn = matches!(composer_txn, ComposerTxn::Transaction(_)); |
| /// cases (otherwise we have a bunch of different code paths in our implementations). We probably could use some unsafe code | ||
| /// (i.e `UnsafeCell`) in place of the `RefCell`, but to make our lives easier and avoid accidental UB we use `RefCell` instead. The | ||
| /// perofmrance difference is likely negligible in practice. | ||
| pub struct FfiMutex<T>(InnerMutex<T>); |
There was a problem hiding this comment.
should we update Cargo.toml make the defaukt feature either uniffi or wasm? Without the default feature, rust analyer isn't happy because it doesn't know what is InnerMutex
Follows up on the spike in #119 but removes the FFI utils crate and packages for now. When working on them I realized that they will take a decent amount of time to implement due do all the new types/traits, so figured it makes more sense to review the rust impl on its own and then later PR the FFI crate and packages