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.

Sign up for GitHub

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

Jump to bottom

Integrate scanResultBlob pattern on robonode-client and bioauth-flow-rpc #1026

Merged
merged 33 commits into from
Sep 17, 2024
Merged

Integrate scanResultBlob pattern on robonode-client and bioauth-flow-rpc #1026

merged 33 commits into from
Sep 17, 2024

Conversation

dmitrylavrenov
Copy link
Contributor

@dmitrylavrenov dmitrylavrenov commented May 7, 2024 •
edited
Loading

Should be merged after #1065

As agreed integration of scanResultBlob pattern on bioauth-flow-rpc and robonode-client sides should be backward compatible.

@dmitrylavrenov dmitrylavrenov changed the title Integrate scanResultBlob used in new facetec versions Integrate scanResultBlob pattern used in new facetec versions May 7, 2024
@dmitrylavrenov dmitrylavrenov marked this pull request as ready for review May 9, 2024 10:37
@dmitrylavrenov dmitrylavrenov requested a review from MOZGIII May 9, 2024 10:38
@MOZGIII
Copy link
Contributor

MOZGIII commented May 10, 2024

This needs to be tested against the corresponding web app implementation before it can be merged. The reason is we don't know if this would work at all.

@dmitrylavrenov
Copy link
Contributor Author

This needs to be tested against the corresponding web app implementation before it can be merged. The reason is we don't know if this would work at all.

Sure, will check.

@dmitrylavrenov dmitrylavrenov mentioned this pull request May 13, 2024
Closed
@dmitrylavrenov dmitrylavrenov changed the title Integrate scanResultBlob pattern used in new facetec versions Integrate scanResultBlob pattern on chain side May 13, 2024
@MOZGIII
Copy link
Contributor

MOZGIII commented May 27, 2024

This PR seems to include both changes for robonode and for the chain, please make it draft and update the base

@dmitrylavrenov
Copy link
Contributor Author

This PR seems to include both changes for robonode and for the chain, please make it draft and update the base

Sure, will make it draft. Just robonode side is here - #1037.

@dmitrylavrenov dmitrylavrenov marked this pull request as draft May 27, 2024 14:05
@dmitrylavrenov dmitrylavrenov changed the base branch from master to scan-result-blob-robonode May 27, 2024 14:06
@dmitrylavrenov dmitrylavrenov force-pushed the scan-result-blob branch 3 times, most recently from 2c59027 to 3189e91 Compare May 28, 2024 19:00
@dmitrylavrenov dmitrylavrenov force-pushed the scan-result-blob-robonode branch from 9fd4204 to a67eeac Compare May 28, 2024 19:01
@dmitrylavrenov dmitrylavrenov force-pushed the scan-result-blob-robonode branch from a67eeac to 1706f9e Compare May 28, 2024 19:10
@dmitrylavrenov dmitrylavrenov force-pushed the scan-result-blob-robonode branch from 1706f9e to ad03a17 Compare May 28, 2024 20:27
@dmitrylavrenov dmitrylavrenov force-pushed the scan-result-blob-robonode branch from ad03a17 to 111e505 Compare May 29, 2024 09:27
dmitrylavrenov and others added 22 commits September 16, 2024 22:09
@dmitrylavrenov
Use V2 terminology
e9f101b
@dmitrylavrenov
Separate errors containing scan result blob on robonode-client
8a0a10e
@dmitrylavrenov
Properly handle errors at bioauth-flow-rpc
5bccb0c
@dmitrylavrenov
Separate V2 related logic
a4c2388
@dmitrylavrenov
Fix tests
5b7d6d3
@dmitrylavrenov
Improve robonode-client errors naming
0110499
@dmitrylavrenov
Move robonode_request errors data to shared errors data
ab0ddce
@dmitrylavrenov
Use shared error explicitly
7a59051
@dmitrylavrenov
Rename shared Robonode error into RobonodeClient error
9b2e027
@dmitrylavrenov
Rename shared::Error into shared::FlowBaseError
88162df
@dmitrylavrenov
Use only a single item
0383c4b
@dmitrylavrenov
Use shared deps directly
42c66fd
@dmitrylavrenov
Avoid no blob in errors messages
d468e79
@dmitrylavrenov
Avoid panics
bff758c
@dmitrylavrenov
Add ShouldRetry for new methods working with old robonode
d53ba8d
@dmitrylavrenov
Explicitly unpack the robonode errors
ef56fb2
@MOZGIII @dmitrylavrenov
Correct the comment on the ScanResultBlob error data
5f9f668 
It lacks the contextual information that we aim to communicate via
ShouldRetry, instead it just contains a scan result blob - so document
accordingly.
@MOZGIII @dmitrylavrenov
Share error serialization code to simplify the mental model
886838b
@dmitrylavrenov @MOZGIII
Simplify error_data usage
50a696f 
Co-authored-by: MOZGIII <[email protected]>
@dmitrylavrenov
Fix logic of old robonode-server empty response at robonode-client side
9507dd2
@dmitrylavrenov @MOZGIII
More compact way to return enroll response
e37b3ba 
Co-authored-by: MOZGIII <[email protected]>
@dmitrylavrenov
Properly use blob related cases in tests
fd80630
@MOZGIII
Copy link
Contributor

MOZGIII commented Sep 16, 2024

Great, we can now proceed with merging and shipping this as usual - to internal testing first, and then on.

dmitrylavrenov
dmitrylavrenov commented Sep 17, 2024
View reviewed changes
Copy link
Contributor Author

@dmitrylavrenov dmitrylavrenov left a comment

Choose a reason for hiding this comment

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

LGTM!

@dmitrylavrenov dmitrylavrenov added this pull request to the merge queue Sep 17, 2024
Merged via the queue into master with commit e6902a7 Sep 17, 2024
21 checks passed
@dmitrylavrenov dmitrylavrenov deleted the scan-result-blob branch September 17, 2024 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sesh92 sesh92 sesh92 left review comments

@MOZGIII MOZGIII MOZGIII approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dmitrylavrenov @MOZGIII @sesh92