feat!: Add new QR version for new session flow#655
Conversation
danielle-tfh
force-pushed
the
make-fields-optional-for-user-data
branch
from
f3a88f8 to
754d012
Compare
danielle-tfh
changed the title
andronat
left a comment
andronat
left a comment
There was a problem hiding this comment.
Sorry i might be too tired but i think this won't work. if apps upgrade to thid version the hash will be calculated differently between app and orb. also if we upgrade the orbs first, we will have the same issue. both chancing fields have issues. i think the only way to roll this out is either by doing 2 hash functions or by 2 releases. let's discuss tomorrow. again i might be completely wrong and my brain hallucinate..
| use serde::{Deserialize, Serialize}; | ||
|
|
||
| const PCP_VERSION_DEFAULT: u16 = 2; | ||
| pub const PCP_VERSION_DEFAULT: u16 = 2; |
There was a problem hiding this comment.
why do we need this public? is it for orb relay messages?
danielle-tfh
force-pushed
the
make-fields-optional-for-user-data
branch
3 times, most recently
from
14c92fb to
a7be680
Compare
danielle-tfh
force-pushed
the
make-fields-optional-for-user-data
branch
from
a7be680 to
34f672d
Compare
I added tests for this in a new commit. It should work with the new changes introduced in both cases. Let me know if i am missing something. I also outlined the roll out here https://www.notion.so/worldcoin/Proposal-Eliminate-Session-Centric-Verification-Flow-2418614bdf8c802893d9c3fe43ebba81 |
danielle-tfh
force-pushed
the
make-fields-optional-for-user-data
branch
from
2d4e2c9 to
1fbce7e
Compare
danielle-tfh
changed the title
| hasher.update(os_version.as_bytes()); | ||
| hasher.update(os.as_bytes()); | ||
| if *pcp_version != PCP_VERSION_DEFAULT { | ||
| hasher.update(&pcp_version.to_ne_bytes()); |
There was a problem hiding this comment.
Hmm actually this is dangerous no? ne is for native endianness and if different platforms have different endianness this will break... We are actually lucky that arm and x86 are little endian by default. Could we do it _le_?
There was a problem hiding this comment.
Nice find! Let me update it
| /// Parses `session_id` and `user_data_hash` from a QR-code string. | ||
| /// This decode version does not support v4 since no v4 should be used with this method | ||
| /// All orbs should be updated to support v4 since it requires specific logic | ||
| pub fn decode_qr(qr: &str) -> Result<(Uuid, Vec<u8>), DecodeError> { |
There was a problem hiding this comment.
I think we should remove this or break the current api. in any case the code doesn't need to be backwards compatible. Only the QR handling needs to. The plan will be we update all the orbs first to have the new v3+v4 decoders everywhere and then we update the apps to generate v4 only QRs.
| } | ||
|
|
||
| /// Generates a QR-code (V4) string from `orb relay id` and `app_authenticated_data` | ||
| pub fn encode_static_qr( |
There was a problem hiding this comment.
Similar to my comments above, we should remove the old encode_qr and just update it with the following code. We no longer need to generate v3s if we move to v4.
There was a problem hiding this comment.
I initially kept it for unit tests for ensuring backwards compatibility with decode.
But if you feel that its not needed - then let me remove
| //! let success = user_data.verify(user_data_hash); | ||
| //! ``` | ||
| //! | ||
| //! # New flow |
There was a problem hiding this comment.
Similar here. Just remove the old examples and make this the canonical one.
danielle-tfh
force-pushed
the
make-fields-optional-for-user-data
branch
from
8ed7a28 to
3b71597
Compare
danielle-tfh
force-pushed
the
make-fields-optional-for-user-data
branch
from
3b71597 to
5121be0
Compare
2 participants
Notes
We are planning a new roll-out of the session validation flow: https://www.notion.so/worldcoin/Proposal-Eliminate-Session-Centric-Verification-Flow-2418614bdf8c802893d9c3fe43ebba81.
This PR prepares a new version of qr codes where the orb knows the version of the QR code