manuelmauro/algonaut

Revisit design around `AlgonautError` and functionality in root package

Closed this issue · 3 comments

We created an abstraction layer for algod/kmd/indexer with an unified error type: AlgonautError.

The are parts of the public API which still use package specific errors like TransactionError. This seems awkward, as AlgonautError suggests that it's the only public error type. It may also lead to confusion in practice, e.g. users write a function that returns AlgonautError (it can be internal convenience around Algonaut, where they doesn't want to convert to other error types yet) and applying ? to the package specific errors will unexpectedly not work.

It may appear that we should have all the public APIs return AlgonautError, but this doesn't seem right, as users would have to unnecessarily handle errors unrelated to the functions they're calling. We could also add From implementations to convert the package specific errors to AlgonautError, which would solve the ? issue, but not the awkward design.

The current AlgonautError as well as the functionality in the root package correspond basically to "clients with convenience": should we create a new package for this, rename it in "client", "service" or similar (and AlgonautError accordingly) (renaming the REST client functionality in something else if needed)?

I think a separate package for convenience and utility functions makes sense, for example, this would be really useful so people don't have to re-implement something so common:

/// Utility function to wait on a transaction to be confirmed
async fn wait_for_pending_transaction(
algod: &Algod,
txid: &str,
) -> Result<Option<PendingTransaction>, AlgonautError> {
let timeout = Duration::from_secs(10);
let start = Instant::now();
loop {
let pending_transaction = algod.pending_transaction_with_id(txid).await?;
// If the transaction has been confirmed or we time out, exit.
if pending_transaction.confirmed_round.is_some() {
return Ok(Some(pending_transaction));
} else if start.elapsed() >= timeout {
return Ok(None);
}
std::thread::sleep(Duration::from_millis(250))
}
}

but it isn't really align with the official SDKs

I'd not be against adding some convenience, if it's used often enough. An issue here however, is that we guarantee WASM compatibility, and to work with WASM, wait_for_pending_transaction needs additional dependencies, which adds bloat to the SDK. This is the updated version:

pub async fn wait_for_pending_transaction(
    algod: &Algod,
    tx_id: &str,
) -> Result<Option<PendingTransaction>, AlgonautError> {
    let timeout = Duration::from_secs(30);
    let start = Instant::now();
    log::debug!("Start waiting for pending tx confirmation..");
    loop {
        let pending_transaction = algod.pending_transaction_with_id(tx_id).await?;
        // If the transaction has been confirmed or we time out, exit.
        if pending_transaction.confirmed_round.is_some() {
            return Ok(Some(pending_transaction));
        } else if start.elapsed() >= timeout {
            log::debug!("Timeout waiting for pending tx");
            return Ok(None);
        }
        sleep(250).await;
    }
}

#[cfg(target_arch = "wasm32")]
pub async fn sleep(ms: u32) {
    gloo_timers::future::TimeoutFuture::new(ms).await;
}

#[cfg(not(target_arch = "wasm32"))]
pub async fn sleep(ms: u32) {
    futures_timer::Delay::new(std::time::Duration::from_millis(ms as u64)).await;
}

If you find a way to do this without third parties we can add it, otherwise I tend towards keeping it outside.

wait_for_pending_transaction is now part of Algonaut, because it's needed for the Atomic Transaction Composer, which is a core feature.