Man the combination of Objective-C and clang-format here isn't very pretty

Is it necessary to make this public?

An equality check seems like it won't work if version is incremented, right?

Technically it isn't. secureStoreEncryptionKeyID is a variable under which we store ID for encryption key in secure store. I needed this ID in AppDelegate in a function where I try to change accessibility options for this key. I could just copy-paste the string but in my opinion this is not the best solution since it increases coupling in the codebase.I agree that making class attribute public is not the best solution either. We have Linear task to tackle this issue: https://linear.app/comm/issue/ENG-585/create-common-place-for-constants. Once this Linear task is done we will probably want to make it private again. Finally answering your question again, this attribute can be private at the cost of copy-pasting the string from SQLiteQueryExecutor. We need to make sure that we eventually create some common place for important string constants, since if we continue to copy-paste them wherever we need we might get into trouble.

I cannot commit changes without running clang-format. Should I spent some time searching for better alternatives for Objective-C code formatting? At Software Mansion we have a team of expo developers who code in Objective-C for iOS. I can ask them about their formatting strategies. I suggest this since most of the time I put a diff in Objective-C code formatting is being complained about. Perhaps it is high time we did something about it.

The purpose of this differential is to delete encryption key from secure store and put it there again, but with new accessibility options (options attribute of shared). Technically we could perform this operations everytime application is foregrounded, but I didn't like this idea so I decided we should mark it somehow that this operation was executed. I was thinking of NSUserDefaults but @tomek suggested not to do so since it will introduce an unexplored dimension along which application could crash and it is better to use secure store for this purpose as we I decided to follow the same pattern we use for SQLite migrations. This way if we ever to decide to modify encryption key settings again we will need to modify options attribute and bump the version argument.

Seems like you could just make it private or package-only (what it was before) and also implement a getter...

I thought about this more...

Usually in migrations code you want to check if the current version is less than the migration version when deciding whether to run the migration. That way, if the current version is lower than the migration version, it will still run.

Three changes I would suggest:

1. In this case, since it is the first migration, it's actually not possible for version to be less than 0. But I still think it is the right paradigm to check if the value is less than or equal to 0.
2. We should use an integer instead of a string for the migration version.
3. The migration version shouldn't be passed into this function... there should be a data structure pointing from 0 to the migration function, where we can add a new migration later if necessary.

It might be a good idea to take a look at existing examples of migrations in our codebase: 1, 2, 3

Why do we have to provide this option when reading?

Under the hood ExpoSecureStore uses SecItemCopyMatching (https://developer.apple.com/documentation/security/1398306-secitemcopymatching?language=objc) which returns items whose attributes sets are supersets of attributes we pass to SecItemCopyMatching. Therefore I thought it will be a good practice (even though not necessary) to pass options we used to put encryption key to a search call. But after diving deeper into _getValueWithKey implementation on the expo side it looks like they do not care about accessibility options (even though they care about them in _setValueWithKey).
In other words - I put options here since apple docs of SecItemCopyMatching encourage to use the same set of attributes for search that was used during insertion. But since expo implementation does not care about options when calling SecItemCopyMatching this is neither necessary nor harmful. Passing options here is a no-op.

If this is no-op, it's better to avoid that. The code that does nothing still has to be maintained.

Regarding the internal implementation, what would happen if we pass incorrect option? Are there any downsides of not specifying the option at all?

If I understand implementation and apple docs correctly - passing incorrect option might result in an item not being found. Not passing any options simply broadens the scope of results returned. But since we do not store many values under key "encryptionKey" not passing any options does not affect the search.

Ok, so an approach where we take this value whatever the accessibility policy is set, is what we need.

Do we have a follow-up Linear task here?

@tomek please tell me if I am right:

Ok, so an approach where we take this value whatever the accessibility policy is set, is what we need.

Do you mean that we want to change the code so that we don't pass options when retrieving a key?

Yes, exactly

Do we have a follow-up Linear task here?

Created here: https://linear.app/comm/issue/ENG-2659/change-commsecurestoreioswrapper-get-method-implementation-not-to-pass