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.

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

chore: remove decentralised verification paths #4274

Merged
merged 25 commits into from
Oct 18, 2023

Conversation

kathaypacific
Copy link
Collaborator

@kathaypacific kathaypacific commented Oct 6, 2023

Description

This PR removes:

  • remote config flag for decentralizedVerificationEnabled
  • stuff related to decentralised phone number lookups with ODIS
  • stuff relating to ODIS quota purchases
  • decentralised verification analytics events which are no longer used
  • decentralised revoke mapping
  • translations relating to decentralised verification - unlikely we'll want to update this
  • fetch lost accounts on firebase

Note: background migration from decentralised verification to CPV was not removed since it seems there's still traffic to that endpoint. This flow will only work if the user has a cached salt for their own number.

Test plan

CI

Related issues

Backwards compatibility

Y

Base automatically changed from kathy/remove-require-cpv to main October 7, 2023 19:03
@emerge-tools
Copy link

emerge-tools bot commented Oct 9, 2023

1 build decreased size

Name Version Download Change Install Change
Celo (test) 1.70.0 (135) 26.4 MB ⬇️ 415.1 kB (-1.55%) 63.3 MB ⬇️ 1.0 MB (-1.61%)

Celo (test) 1.70.0 (135)

⚖️ Compare build
📦 Install build
⏱️ Analyze build performance

Total install size change: ⬇️ 1.0 MB (-1.61%)
Total download size change: ⬇️ 415.1 kB (-1.55%)

Largest size changes

Item Install Size Change
main.jsbundle ⬇️ -380.9 kB
🗑 celo ⬇️ -57.0 kB
🗑 celo ⬇️ -33.2 kB
🗑 celo ⬇️ -27.8 kB
🗑 celo ⬇️ -20.1 kB

Image of diff


🛸 Powered by Emerge Tools

@codecov
Copy link

codecov bot commented Oct 9, 2023

Codecov Report

Merging #4274 (db85c58) into main (ad1a323) will increase coverage by 0.21%.
The diff coverage is 88.46%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4274      +/-   ##
==========================================
+ Coverage   84.06%   84.28%   +0.21%     
==========================================
  Files         712      707       -5     
  Lines       26319    25972     -347     
  Branches     3445     3427      -18     
==========================================
- Hits        22126    21891     -235     
+ Misses       4127     4015     -112     
  Partials       66       66              
Files Coverage Δ
src/account/Settings.tsx 85.79% <ø> (+2.80%) ⬆️
src/analytics/Events.tsx 100.00% <ø> (ø)
src/app/ErrorMessages.ts 100.00% <ø> (ø)
src/app/reducers.ts 20.83% <ø> (ø)
src/app/saga.ts 74.02% <100.00%> (+0.89%) ⬆️
src/firebase/firebase.ts 38.77% <ø> (+1.21%) ⬆️
src/firebase/remoteConfigValuesDefaults.ts 100.00% <ø> (ø)
src/identity/actions.ts 83.92% <ø> (-12.74%) ⬇️
src/identity/privateHashing.ts 83.33% <100.00%> (+22.22%) ⬆️
src/identity/reducer.ts 32.14% <100.00%> (-0.07%) ⬇️
... and 12 more

... and 5 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad1a323...db85c58. Read the comment docs.

@kathaypacific kathaypacific marked this pull request as ready for review October 10, 2023 11:33
Copy link
Member

@jeanregisser jeanregisser left a comment

Choose a reason for hiding this comment

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

Awesome, this feels good! ✂️ 💥

Comment on lines -16 to -17
REVOKE_VERIFICATION = 'IDENTITY/REVOKE_VERIFICATION',
REVOKE_VERIFICATION_STATE = 'IDENTITY/REVOKE_VERIFICATION_STATE',
Copy link
Member

Choose a reason for hiding this comment

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

Are there more actions we can remove around here?
Seeing:

  • SET_SEEN_VERIFICATION_NUX
  • FETCH_ADDRESSES_AND_VALIDATION_STATUS
  • UPDATE_WALLET_TO_ACCOUNT_ADDRESS

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

these all seem to be used...am i missing something?

  • SET_SEEN_VERIFICATION_NUX seems to be used for determining the initial route (the name is confusing but seems more like a "HAS_SKIPPED_PHONE_VERIFICATION" action)
  • FETCH_ADDRESSES_AND_VALIDATION_STATUS seems to be used for the phone number lookup
  • UPDATE_WALLET_TO_ACCOUNT_ADDRESS is used to fetch the DEK
    i'm happy to remove any of these if we don't need the flows anymore but the code paths look still alive for these

Copy link
Member

Choose a reason for hiding this comment

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

ahah ok, well was just trying to see if there were more actions we could now remove :D

})
})
}
function* navigateToQuotaPurchaseScreen() {
Copy link
Member

Choose a reason for hiding this comment

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

Good to see this one gone.

@jeanregisser
Copy link
Member

jeanregisser commented Oct 13, 2023

@kathaypacific I was surprised to see we still get successful migrations after all this time. About 97 in the last 30 days.
Maybe we should keep the migration for now, for users who have their ODIS pepper cached?

Copy link
Member

@jeanregisser jeanregisser left a comment

Choose a reason for hiding this comment

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

Great! 🚀

@kathaypacific kathaypacific enabled auto-merge (squash) October 18, 2023 07:15
@kathaypacific kathaypacific merged commit cd5ffce into main Oct 18, 2023
15 checks passed
@kathaypacific kathaypacific deleted the kathy/remove-decentralized-verification branch October 18, 2023 07:49
bakoushin pushed a commit that referenced this pull request Oct 18, 2023
### Description

This PR removes:
- remote config flag for decentralizedVerificationEnabled
- stuff related to decentralised phone number lookups with ODIS
- stuff relating to ODIS quota purchases
- decentralised verification analytics events which are no longer used
- decentralised revoke mapping
- translations relating to decentralised verification - unlikely we'll
want to update this
- fetch lost accounts on firebase

Note: background migration from decentralised verification to CPV was
not removed since it seems there's still traffic to that endpoint. This
flow will only work if the user has a cached salt for their own number.

### Test plan

CI

### Related issues

- Fixes RET-880

### Backwards compatibility

Y

---------

Co-authored-by: Jean Regisser <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants