-
Notifications
You must be signed in to change notification settings - Fork 76
feat: add get_messages_status #776
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| use mc_settlement_client::error::SettlementClientError; | ||
| use mp_rpc::v0_8_1::{L1TxnHash, TxnHashWithStatus}; | ||
|
|
||
| use crate::{ | ||
| utils::display_internal_server_error, | ||
| versions::user::v0_7_1::methods::read::get_transaction_status::get_transaction_status, Starknet, | ||
| StarknetRpcApiError, StarknetRpcResult, | ||
| }; | ||
|
|
||
| pub async fn get_messages_status( | ||
| starknet: &Starknet, | ||
| transaction_hash: L1TxnHash, | ||
| ) -> StarknetRpcResult<Vec<TxnHashWithStatus>> { | ||
| let transaction_hash = transaction_hash.into(); | ||
| let settlement_client = starknet.settlement_client().ok_or(StarknetRpcApiError::InternalServerError)?; | ||
| let l1_handlers = settlement_client.get_messages_to_l2(transaction_hash).await.map_err(|e| match e { | ||
| SettlementClientError::TxNotFound => StarknetRpcApiError::TxnHashNotFound, | ||
| e => { | ||
| display_internal_server_error(format!( | ||
| "Error getting messages to L2 for L1 transaction {transaction_hash:?}: {e}" | ||
| )); | ||
| StarknetRpcApiError::InternalServerError | ||
| } | ||
| })?; | ||
|
|
||
| let mut results = Vec::new(); | ||
| for msg in l1_handlers { | ||
| let transaction_hash = msg.tx.compute_hash(starknet.chain_id(), false, false); | ||
| let finality_status = get_transaction_status(starknet, transaction_hash).await?.finality_status; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think we're supposed to return something meaningful when the transaction is not on l2 here
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alright - I am looking at the latest v0.9 spec. I also looked at the v0.8 rpc spec but I figured I would focus on v0.9 spec because we'll have to do that as well, and the corner cases are similar. Here is the interpretation of the spec.
I think I need to bring this up to the spec authors, but in the meantime, my opinion is that you should:
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this implementation is only for v0.8.1 |
||
| // TODO: Add failure_reason when we support it in get_transaction_status in v0.8.1. | ||
| results.push(TxnHashWithStatus { transaction_hash, finality_status, failure_reason: None }); | ||
| } | ||
| Ok(results) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh btw,
should work
(https://docs.rs/futures/latest/futures/stream/index.html, and https://docs.rs/futures/latest/futures/stream/trait.StreamExt.html#method.then which is the async version of iterator
map)(it doesn't change anything, the fetches will still be sequential but i think it looks nicer? maybe overcomplicated for nothing idk)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there’s a slight overhead with the stream I think, and I’m not sure there’s more than 1 message per TX