Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 support for handling billing process for the service offered by the operator #18
base: master
Are you sure you want to change the base?
Add support for handling billing process for the service offered by the operator #18
Changes from 16 commits
6f6aefd
bab98e6
2f2fd1e
d8ecd7f
b67ca15
a295980
0295563
a8a0ee7
4502fbc
2ef0b87
350d1cf
bd80202
409d90f
1a47140
d59dc0c
d317e86
f21eee6
66703c9
42a3d4c
b4f8a3a
6fe2e1b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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.
If signature is failing here, then it will fail again when
billing/latest
will be called. There is no change in parameters or other context data. Signature failing here seems fatal error to me. Double check where it can fail and how developers has suggested to handle those errors.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 looked into the error conditions and possibilities while signing prehash messages and saw if we're using a valid secret scalar key for signing (that I suppose we must be using) and a valid hashing algorithm that gives out a 32-byte hash (which is the case for our
Keccak::v256()
hasher) then it is very rare that an error would occur. Have added the error checks just in case something weird happens (because of outdated crate version or something).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.
Btw If we look more closely, there is an aggressive exit happening when serving the requests as well after failing to sign the requests/response data. So even after getting a successful response from the worker serving the app, we're throwing an error response to the user of the app. Is this an acceptable behaviour? @roshanr95 @vg-27
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.
So, if signing is throwing exception then it will be there for every sign call. That means this application is not usable at all in that case, in production.
Then I think, if you just return exception here, that would be sufficient. Any exception due to implementation change, would be hit during testing phase of release.
You can remove the special handling here, it looks like you are trying to recover from error, but it's not. Kinda misleading.
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 was just handling it the way it was done when the serverless requests are getting processed by the enclave. I think that is more concerning since it is part of the code already in production. So shouldn't we add an exception here as well:-
pub async fn get_workerd_response(
) -> Result<HttpResponse, anyhow::Error> {
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.
Here in this case
?
is used to pass on exception. No explicit handling required. Even for your case, I was suggesting just respond back with InternalError. And that would be fine.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.
Depends on why it can fail and if it's consistent or not. For example, pretty sure it generates a random number inside, which can probably fail inconsistently.
Storing the last response is mainly just a safeguard I guess, if we don't even want to spend time really figuring out the failure conditions.
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.
Yes, responses are insecure without a signature. No point in returning a response if not signed.
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.
Yeah, that's right. Plain ECDSA signature uses random number. But here implementation seems to be using RFC6979, where
k
is computed deterministically.Anyways, if there is possibility of inconsistent error. Let's keep the backup of billing data.
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.
Is it safe to assume that nonce is supposed to be unique? If yes, then can we save this data with given nonce as key. So that if in a case, client loses the bill, it can query bill again by providing the nonce. And maybe delete the cache after some timeout.
To avoid the case, where client got disconnected from enclave.
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.
Yes nonce is supposed to be unique based on which the contract will catch duplication of bill data. We can store this data in cache for some time but I think once the client receives the data, they are supposed to claim the bill first thing and even if not that they are atleast supposed to store the bill data until they do. I don't know if we should populate the enclave with more data and affect their performance or capacity just to handle this. @roshanr95 Please comment.
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.
Yes, the contract will deduplicate receipts based on the nonce
Storing all, or even some, of them is probably overkill, can maybe just store the very last one that the client can then requery (
/billing/latest
). Assuming proper persistence (and API usage) is the responsibility of the client, the only real loss vector that remains is network issues while the response is being transmitted, just the last one should be fine to handle this.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.
We can also protect against duplicate requests by checking the nonce and seeing if it has changed since the last 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.
Why is this required inside the enclave? Our major aim is to not allow duplicate requests to the billing contract because that's where the actual transactions happen. If client is sending a duplicate nonce to the enclave then it's their fault because then the enclave will think some balances have been settled when in fact the contract will reject it ,leading to lost bill info. If we were to check nonce inside the enclave then we'll have to check inside the list of all received yet and not just the last one which is probably an overkill.
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.
Agree. Nonce check should also be enforced. Also storing the last bill is sufficient to handle the network issue.