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

Add payload parsing to hyperlane Rest API #13

Merged

Conversation

luke-lorenzini
Copy link

@luke-lorenzini luke-lorenzini commented Nov 21, 2024

Description

Added rest_client.rs to project to interact with Sovereign rollup.

(PR will be squashed before merge)

Drive-by changes

No

Related issues

NA

Backward compatibility

Yes

Testing

Extensive manual testing. Used for all demo and dev purposes.

@luke-lorenzini luke-lorenzini changed the title Add payload parsing to hl rest api Add payload parsing to hyperlane Rest API Nov 26, 2024
@luke-lorenzini luke-lorenzini force-pushed the add-payload-parsing-to-hl-rest-api branch from c3731c7 to dc7eb43 Compare December 10, 2024 17:57
Copy link
Member

@asmie asmie left a comment

Choose a reason for hiding this comment

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

👍

rust/main/chains/hyperlane-sovereign/src/indexer.rs Outdated Show resolved Hide resolved
rust/main/chains/hyperlane-sovereign/src/indexer.rs Outdated Show resolved Hide resolved
Comment on lines 77 to 83
let mut results = Vec::new();

tx.events
.iter()
.filter(|e| e.key == Self::EVENT_KEY)
.try_for_each(|e| -> ChainResult<()> {
let (indexed_msg, meta) = self.process_event(tx, e, tx.batch_number, batch_hash)?;
info!(
"Processed {} event : {:?} - Meta: {:?}",
Self::EVENT_KEY,
indexed_msg,
meta
);
results.push((indexed_msg, meta));
Ok(())
})?;
Ok(results)
Copy link
Member

Choose a reason for hiding this comment

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

So, would something like the below code work?
Removed the intermediate results vector and used collect() to create the result vector directly from the iterator. Would it be a bit faster to avoid pushing each element to a vector?

Suggested change
let mut results = Vec::new();
tx.events
.iter()
.filter(|e| e.key == Self::EVENT_KEY)
.try_for_each(|e| -> ChainResult<()> {
let (indexed_msg, meta) = self.process_event(tx, e, tx.batch_number, batch_hash)?;
info!(
"Processed {} event : {:?} - Meta: {:?}",
Self::EVENT_KEY,
indexed_msg,
meta
);
results.push((indexed_msg, meta));
Ok(())
})?;
Ok(results)
tx.events
.iter()
.filter(|e| e.key == Self::EVENT_KEY)
.map(|e| {
let (indexed_msg, meta) = self.process_event(tx, e, tx.batch_number, batch_hash)?;
info!(
"Processed {} event: {:?} - Meta: {:?}",
Self::EVENT_KEY,
indexed_msg,
meta
);
Ok((indexed_msg, meta))
})
.collect()

let thingy = self.decode_event(event)?;

let meta = LogMeta {
address: batch_hash, //TODO!!! this is wrong
Copy link
Member

Choose a reason for hiding this comment

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

Why it's wrong?

Copy link
Member

Choose a reason for hiding this comment

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

Still IDK why it's wrong 😸 Maybe it should be explained in the comment?

@luke-lorenzini luke-lorenzini marked this pull request as ready for review February 2, 2025 21:56
Copy link
Member

@asmie asmie left a comment

Choose a reason for hiding this comment

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

👍

let thingy = self.decode_event(event)?;

let meta = LogMeta {
address: batch_hash, //TODO!!! this is wrong
Copy link
Member

Choose a reason for hiding this comment

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

Still IDK why it's wrong 😸 Maybe it should be explained in the comment?

@asmie
Copy link
Member

asmie commented Feb 3, 2025

Please make sure all public structs and functions are properly documented.

@luke-lorenzini luke-lorenzini force-pushed the add-payload-parsing-to-hl-rest-api branch 13 times, most recently from d5017b3 to cf6c6c0 Compare February 4, 2025 00:04
@luke-lorenzini luke-lorenzini marked this pull request as draft February 4, 2025 00:15
@luke-lorenzini luke-lorenzini force-pushed the add-payload-parsing-to-hl-rest-api branch 7 times, most recently from 9ed826d to 27a8b6e Compare February 5, 2025 01:23
@luke-lorenzini luke-lorenzini marked this pull request as ready for review February 5, 2025 02:38
Copy link
Member

@asmie asmie left a comment

Choose a reason for hiding this comment

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

Let's merge it! 👍

Comment on lines +478 to +493
// // /modules/bank/tokens/{token_id}/balances/{address}
// let query = format!("/modules/bank/tokens/{}/balances/{}", token_id, address);

// #[derive(Clone, Debug, Deserialize)]
// struct Data {
// _amount: Option<u128>,
// _token_id: Option<String>,
// }

// let response = self
// .http_get(&query)
// .await
// .map_err(|e| ChainCommunicationError::CustomError(format!("HTTP Get Error: {}", e)))?;
// let response: Schema<Data> = serde_json::from_slice(&response)?;

// let response = U256::from(response);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe delete those comments?

@luke-lorenzini luke-lorenzini force-pushed the add-payload-parsing-to-hl-rest-api branch from 27a8b6e to b75175d Compare February 5, 2025 20:21
@luke-lorenzini luke-lorenzini force-pushed the add-payload-parsing-to-hl-rest-api branch from b75175d to 9945b9d Compare February 5, 2025 21:00
@luke-lorenzini luke-lorenzini merged this pull request into hyperlane-sovereign Feb 5, 2025
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.

2 participants