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

feat(ios): wrapper implementation #11

Merged
merged 2 commits into from
Jan 24, 2025
Merged

feat(ios): wrapper implementation #11

merged 2 commits into from
Jan 24, 2025

Conversation

berendsliedrecht
Copy link
Member

@berendsliedrecht berendsliedrecht commented Jan 22, 2025

  • ios wrapper full implementation for RN >= 75. (old arch tested only right now)

@berendsliedrecht berendsliedrecht force-pushed the ios-rn-mdoc branch 4 times, most recently from 8b953ba to df8a37e Compare January 22, 2025 09:58
Signed-off-by: Berend Sliedrecht <[email protected]>

if defined?(:spm_dependency)
spm_dependency(s,
url: 'https://github.com/berendsliedrecht/eudi-lib-ios-iso18013-data-transfer.git',
Copy link
Member

Choose a reason for hiding this comment

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

Might be good to add a comment with reason and link to the code: https://github.com/berendsliedrecht/eudi-lib-ios-iso18013-data-transfer/tree/send-device-response-manually

Also -- maybe good to clean up the formatting in that branch a bit so it's easy to see what changed compared to the EUDI wallet implementation.

@@ -9,13 +10,14 @@
"prebuild": "expo prebuild --no-install"
},
"dependencies": {
"@animo-id/expo-mdoc-data-transfer": "../",
"@animo-id/expo-mdoc-data-transfer": "link:../",
Copy link
Member

Choose a reason for hiding this comment

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

Does react native support linked packages? I didn't think so?

Copy link
Member Author

Choose a reason for hiding this comment

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

well it works, so I guess it does?

Copy link
Member

Choose a reason for hiding this comment

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

It doesn't work with pnpm?

Copy link
Member Author

Choose a reason for hiding this comment

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

No there was a weird circular reference between the example app and library. Can be fixed by moving to a workspace I guess.

@objc
func initialize() -> String {
guard bleServerTransfer == nil else {
return ""
Copy link
Member

Choose a reason for hiding this comment

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

This should throw an error

Copy link
Member Author

Choose a reason for hiding this comment

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

DO you mean return an error string? I not mind it too much, but reinitialization calls can happen quite a bit with RN and that would make the UX quite annoying.

Comment on lines 27 to 44
func initialize() -> String {
guard bleServerTransfer == nil else {
return ""
}

do {
bleServerTransfer = try MdocGattServer(parameters: [
InitializeKeys.document_json_data.rawValue: [],
InitializeKeys.trusted_certificates.rawValue: [],
InitializeKeys.send_response_manually.rawValue: true,
])
bleServerTransfer?.delegate = self
} catch {
return error.localizedDescription
}

return ""
}
Copy link
Member

Choose a reason for hiding this comment

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

Why return an empty string? on iOS it returns null

Copy link
Member Author

Choose a reason for hiding this comment

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

its void on Android and a string on iOS, I don't understand what you mean?

Copy link
Member

Choose a reason for hiding this comment

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

On android it's null? Returning empty string is just weird api IMO.

Comment on lines +77 to +80
let cipherData = try sessionEncryption.encrypt(byteArray)
let sd = SessionData(cipher_data: cipherData, status: 20)
try bleServerTransfer.sendResponse(
Data(sd.encode(options: CBOROptions())))
Copy link
Member

Choose a reason for hiding this comment

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

Is this BLE session encryption or mdoc specific encryption? Does the android library also handle this? or is that handled outside of this library (e.g. in mdoc library?)

Copy link
Member Author

Choose a reason for hiding this comment

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

This is handled exactly like the android side. I can also move this to the library.

rejector = nil
resolver = nil

return ""
Copy link
Member

Choose a reason for hiding this comment

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

again not sure on the empty string?

Copy link
Member Author

Choose a reason for hiding this comment

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

empty string = no error
string = error

Could not find another pattern that works. Open to improvements in a follow up PR.

Copy link
Member

Choose a reason for hiding this comment

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

nil is no error?

pnpm-lock.yaml Outdated
Copy link
Member

Choose a reason for hiding this comment

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

no pnpm supported?

Copy link
Member Author

Choose a reason for hiding this comment

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

nope, not yet.

const usageDescription = props?.ios?.bluetoothDescription || defaultDescription

if (c.ios?.infoPlist) {
c.ios.infoPlist.NSBluetoothAlwaysUsageDescription = usageDescription
Copy link
Member

Choose a reason for hiding this comment

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

this could already have been set by the app config? Maybe we should only set it if not already set?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've seen this pattern multiple times to provide it to the package, don't really see the issue with doing it like this. It is optional anyways.

Copy link
Member

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

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

Very nice!! Now that we have a working example we can start looking at improvements later like new arch, and expo support

@berendsliedrecht
Copy link
Member Author

Very nice!! Now that we have a working example we can start looking at improvements later like new arch, and expo support

Yeah. However I would want to wait until it is slightly more stable again... react native, and expo, like to push broken stuff which makes being compatible with the latest version a pain.....

Signed-off-by: Berend Sliedrecht <[email protected]>
@berendsliedrecht berendsliedrecht merged commit a131ebf into main Jan 24, 2025
2 checks passed
@berendsliedrecht berendsliedrecht deleted the ios-rn-mdoc branch January 24, 2025 11:51
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