GaloyMoney / blink-mobile

The Everyday Bitcoin Wallet
https://www.blink.sv
MIT License
138 stars 124 forks source link

react-native-keychain #848

Open nicolasburtey opened 1 year ago

nicolasburtey commented 1 year ago

it seems to no longer be used

2624789 commented 4 months ago

@nicolasburtey I've been going through the app authentication logic and also looking where is keychain already used.

I saw that app/utils/storage/secureStorage.ts uses react-native-secure-key-store but DeviceAccountModal is using react-native-keychain.

I would lean to use react-native-keychain because react-native-secure-key-store last update was two years ago. Although, react-native-keychain use is limited to the DeviceAccountModal component, while react-native-secure-key-store is used more broadly in the app through the KeyStoreWrapper.

I would like to know your opinion about which library would be a better fit to store the token using keychain. Do you think it is a good idea to extend the existing KeyStoreWrapper and keep using react-native-secure-key-store? Another alternative would be to refactor KeyStoreWrapper to use react-native-keychain and unify the use of keychain in the app through this class.

nicolasburtey commented 4 months ago

I have limited context in my head on where we are with the keystore right now. maybe @UncleSamtoshi have some more context to share.

You should decide which trade off you think are better, but I think we generally want to optimize for maintainability. having less library to handle is generally beneficial.

nicolasburtey commented 4 months ago

another key aspect to keep in mind is to the upgrade path for existing user. we don't want them to having to log back in when we do this upgrade

2624789 commented 3 months ago

I agree that for maintainability it is better to have just one library. I'll focus on the feature of this issue that is storing the token on the keychain/keystore without users having to log back. The easiest way I find now for doing this would be to use react-native-secure-key-store.

Removing one of the two libraries and improve maintainability could be a good task for another issue.