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

feat: implement batching #2291

Open
wants to merge 25 commits into
base: master
Choose a base branch
from
Open

feat: implement batching #2291

wants to merge 25 commits into from

Conversation

justprosh
Copy link
Member

No description provided.

@justprosh justprosh added the e2e Run e2e workflow label Jun 19, 2024
crates/chain-data/src/block_header.rs Outdated Show resolved Hide resolved
crates/chain-data/src/block_header.rs Outdated Show resolved Hide resolved
@@ -806,6 +816,14 @@ impl ChainConnector for HttpChainConnector {
}
}

fn parse_str_field(value: Option<Value>, field: &str) -> eyre::Result<String> {
value
.ok_or(eyre!("Field {} not found in response", field))?
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be .ok_or_else(|| eyre!(...)) everywhere, probably.

Copy link
Contributor

Choose a reason for hiding this comment

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

it should be enum without eyre)

Copy link
Contributor

Choose a reason for hiding this comment

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

Even better.

}
// } else if let Err(err) = self.submit_mocked_proofs().await {
Copy link
Contributor

Choose a reason for hiding this comment

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

Commented code.

.and_modify(|c| {
*c = c.add(Uint::from(1));
})
.or_insert(Uint::from(1)),
Copy link
Contributor

@monoid monoid Jul 4, 2024

Choose a reason for hiding this comment

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

IMHO

let proof_something = self.proof_counter.entry(cu_id).or_insert(Uint::ZERO);
*proof_something = *proof_something.add(Uint::from(1));
let proof_id = Uint::from(*proof_something);

would be more explicit.

Copy link
Contributor

Choose a reason for hiding this comment

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

The top-level Uint::from, is it really needed?

let local_nonce = LocalNonce::random();
self.submit_proof(CCProof::new(proof_id, local_nonce, unit, result_hash))
.await?;
// /// Submit Mocked Proofs for all active compute units.
Copy link
Contributor

Choose a reason for hiding this comment

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

More commented code.

tracing::debug!(target: "chain-listener", "Polling proofs after {:?}", last_known_proofs);
measured_request(
&self.metrics,
async { ccp_client.get_proofs_after(last_known_proofs, PROOF_POLL_LIMIT) }
Copy link
Contributor

Choose a reason for hiding this comment

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

The async { X }.await construction may be replaced by just X. Here and elsewhere.

@@ -11,6 +11,10 @@ on:
description: "Cargo dependencies map"
type: string
default: "null"
secrets:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you change that in this pr?

Copy link
Member Author

Choose a reason for hiding this comment

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

it was done by @enjenjenje because of private repo deps

@@ -468,25 +473,28 @@ impl HttpChainConnector {
gas_limit: Uint256::from_le_bytes(&gas_limit.to_le_bytes_vec()),
to: to.parse()?,
value: 0u32.into(),
data,
data: data.clone(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Data can be a pretty big, probably it is better to apply hex::decode before this line and do not use cloning ?

}

async fn persist(&self) {
let backoff = ExponentialBackoff {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need a backoff for the persistence ?

.ok_or(eyre::eyre!("number field not found; got {json}"))?
.to_string();

Ok(Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

The type may be deserialized from a Value with serde in more declarative manner.
https://docs.rs/serde_json/latest/serde_json/fn.from_value.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e Run e2e workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants