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

Revisit design around AlgonautError and functionality in root package #99

Closed
ivnsch opened this issue Aug 12, 2021 · 3 comments · Fixed by #153
Closed

Revisit design around AlgonautError and functionality in root package #99

ivnsch opened this issue Aug 12, 2021 · 3 comments · Fixed by #153

Comments

@ivnsch
Copy link
Contributor

ivnsch commented Aug 12, 2021

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)?

@ivnsch ivnsch added this to the 0.4 milestone Aug 12, 2021
@epequeno
Copy link
Contributor

epequeno commented Nov 9, 2021

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

@ivnsch
Copy link
Contributor Author

ivnsch commented Nov 10, 2021

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.

@ivnsch
Copy link
Contributor Author

ivnsch commented Apr 27, 2022

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

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 a pull request may close this issue.

2 participants