-
Notifications
You must be signed in to change notification settings - Fork 19
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: add value check #153
base: main
Are you sure you want to change the base?
Conversation
Pull Request Test Coverage Report for Build 10946389314Details
💛 - Coveralls |
Ok(()) | ||
} else { | ||
return Err(anyhow!( | ||
"Query receipt does not have the minimum value. Expected value: {}. Minimum value: {}.", |
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.
"Query receipt does not have the minimum value. Expected value: {}. Minimum value: {}.", | |
"Query receipt does not have the minimum value. Expected value: {}. Received value: {}.", |
let request = | ||
serde_json::from_slice(&body).map_err(|e| IndexerServiceError::InvalidRequest(e.into()))?; | ||
|
||
let mut attestation_signer: Option<AttestationSigner> = None; | ||
|
||
if let Some(receipt) = receipt.into_signed_receipt() { | ||
let allocation_id = receipt.message.allocation_id; | ||
let signature = receipt.signature; |
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.
I don't understand how/why you're using the signature
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.
Signature is the best "unique id" for a specific receipt. It is unique for a given receipt + signer.
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.
I thought you wanted it to be unique per "SQL text" vs. unique per each query.
|
||
// get agora model for the allocation_id | ||
let mut cache = self.cost_model_cache.lock().unwrap(); | ||
// on average, we'll have zero or one model |
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.
I'd expect that we can get more than 1 model in the case the model was recently updated, and we are still in the period where expect the query to be priced according to a previous model or the new one.
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.
indeed, we'll have more than one model when recently updated. But I expect that this is not that often. That's the reason for my comment. On average we'll have one model, or even zero if it's not set.
Having the cache in the indexer-service raises a small risk though. |
Signed-off-by: Gustavo Inacio <[email protected]>
Signed-off-by: Gustavo Inacio <[email protected]>
02dc7f2
to
e139644
Compare
Signed-off-by: Gustavo Inacio <[email protected]>
Signed-off-by: Gustavo Inacio <[email protected]>
No description provided.