-
Notifications
You must be signed in to change notification settings - Fork 6
Add account nonce management tracking in server state (RUN-75) #271
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 account nonce management tracking in server state (RUN-75) #271
Conversation
9ab8bcf to
7e07c20
Compare
credential-verification-service/src/api/verification_request.rs
Outdated
Show resolved
Hide resolved
credential-verification-service/src/api/verification_request.rs
Outdated
Show resolved
Hide resolved
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 think this PR is covering two issues. The nonce management and the verification request API. And which parts of the request API is done/final and can be reviewed and which are just temporary implementations? I think to implement nonce management, you can just take an empty request with no fields in the endpoint and crate and submit some dummy verificaiton request and VRA. To keep tasks separate and not mix them together. Then you don't need the VerificationRequestParams in this PR but can leave it to the PR where it naturally belongs.
squirer
left a 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.
Just added some small comments about the request timeout too but all looks good for the most part, nice stuff
credential-verification-service/src/api/verification_request.rs
Outdated
Show resolved
Hide resolved
| // since it is possible that API requests come in parallel. The nonce is | ||
| // increased by 1 and its lock is released after the transaction is submitted to | ||
| // the blockchain. | ||
| let mut account_sequence_number = state.nonce.lock().await; |
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 think we need a timeout on this async operation (locking), like we have for calling the node.
allanbrondum
left a 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.
Still have some comments about the timeouts.
allanbrondum
left a 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.
Still have some comments about the timeouts
486e8c2
into
feature/RUN-36-credential-verification-service
Purpose
https://linear.app/concordium/issue/RUN-75/create-mechanism-to-manage-the-account-sequence-number
https://linear.app/concordium/issue/RUN-78/implement-create-verification-request-endpoint
The full logic of the
/verifiable-presentations/create-verification-requestAPI endpoint (except thepublic_infofield) flow has been implemented. This was done so the account nonce management tracking can be applied and tested on the/verifiable-presentations/create-verification-requestflow.The account nonce management can be applied in the same way on the other anchor flow in the future (not part of this PR).
Example command to call the endpoint:
Changes
/verifiable-presentations/create-verification-requestapi endpoint.