-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Add APIs for non-commissioning device attestation to Matter.framework. #28595
base: master
Are you sure you want to change the base?
Add APIs for non-commissioning device attestation to Matter.framework. #28595
Conversation
PR #28595: Size comparison from a598efd to cf57642 Increases (6 builds for bl602, bl702, nrfconnect, telink)
Decreases (10 builds for efr32, esp32, psoc6, telink)
Full report (60 builds for bl602, bl702, bl702l, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
}); | ||
} | ||
|
||
// Sure would be nice to have await/async. Try to avoid infinite completion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a more conventional way to avoid deep nesting would be to have a helper class holding the state for your async operation, and a separate method per step that looks something like
- (void)fetchVendorID {
[something doSomethingWithCompletion:^(...) {
if (error) {
[self handleError:error];
} else {
_vendorId = result;
[self fetchProductID];
}
}];
}
- (void)verifyAttestationWithQueue:(dispatch_queue_t)queue completion:(MTRDeviceAttestationVerificationHandler)completion | ||
{ | ||
auto * baseDevice = [self newBaseDevice]; | ||
[baseDevice verifyAttestationWithQueue:queue completion:completion]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If going with the helper class approach it could be directly created here be given the MTRBaseDevice, rather than having the code itself live inside MTRBaseDevice.
- (instancetype)initWithDeviceAttestationChallenge:(NSData *)challenge | ||
nonce:(NSData *)nonce | ||
elementsTLV:(MTRTLVBytes)elementsTLV | ||
elementsSignature:(NSData *)elementsSignature | ||
deviceAttestationCertificate:(MTRCertificateDERBytes)deviceAttestationCertificate | ||
productAttestationIntermediateCertificate:(MTRCertificateDERBytes)processAttestationIntermediateCertificate | ||
certificationDeclaration:(NSData *)certificationDeclaration | ||
firmwareInfo:(NSData *)firmwareInfo; | ||
firmwareInfo:(NSData *)firmwareInfo | ||
MTR_NEWLY_DEPRECATED("Please use the version with basicInformationVendorID and basicInformationProductID"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this class an output of device attestation? If so, why is the initializer even public API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If so, why is the initializer even public API?
Because someone did that, we did insufficient review, an then we shipped it... I could make the new initializer not be public, @ksperling-apple
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah just deprecate the current one without a replacement and make the new one internal?
Co-authored-by: Karsten Sperling <113487422+ksperling-apple@users.noreply.github.com>
Co-authored-by: Karsten Sperling <113487422+ksperling-apple@users.noreply.github.com>
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
I still need to get back to the review comments on this and get it updated. |
Fixes #24709