`ARTCipherParams` and `ARTCrypto` doc comments #1439

maratal · 2022-06-30T21:16:58Z

Added docstrings according to ably/sdk-api-reference#8

Was checked for compatibility with jazzy.

maratal · 2022-06-30T23:14:46Z

https://sdk.ably.com/builds/ably/ably-cocoa/pull/1439/jazzydoc/Classes/ARTCrypto.html

lawrence-forooghian · 2022-07-05T20:12:37Z

I've added @m-hulbert as a reviewer here, too, since he might be picking up any necessary changes to the canonical documentation once @tbedford rolls off this project.

lawrence-forooghian · 2022-07-05T20:20:27Z

Source/ARTCrypto.h

@@ -28,13 +28,20 @@ NS_ASSUME_NONNULL_BEGIN
 @end

 /**
- ARTCipherParams contains configuration options for a channel cipher, including algorithm, mode, key length and key.
- Ably client libraries currently support AES with CBC, PKCS#7 with a default key length of 256 bits. All implementations also support AES128.
+ The ARTCipherParams object contains properties to configure encryption for a channel.


Suggested change

The ARTCipherParams object contains properties to configure encryption for a channel.

The `ARTCipherParams` object contains properties to configure encryption for a channel.

@tbedford @m-hulbert I’d suggest that this sort of formatting should be in the canonical documentation, what do you think?

Agree with the formatting. It is already styled as code in the canonical docs though.

This will link to the page that user is already on. Is this what was expected? @lawrence-forooghian

lawrence-forooghian · 2022-07-05T20:22:27Z

Source/ARTCrypto.h

 */
 @interface ARTCipherParams : NSObject <ARTCipherParamsCompatible>
+
+/// The algorithm to use for encryption. Only AES is supported.


@tbedford @m-hulbert For consistency with the documentation for mode, should this not be the following?

Suggested change

/// The algorithm to use for encryption. Only AES is supported.

/// The algorithm to use for encryption. Only `"AES"` is supported.

This should just be AES (no " ") as per the canonical table.

lawrence-forooghian · 2022-07-05T20:23:42Z

Source/ARTCrypto.h

 @property (readonly, nonatomic) NSUInteger keyLength;
+
+/// The cipher mode. Only "CBC" is supported.


Suggested change

/// The cipher mode. Only "CBC" is supported.

/// The cipher mode. Only `"CBC"` is supported.

@tbedford @m-hulbert as I mentioned elsewhere in this PR, I think this sort of formatting should be part of the canonical docs?

This should just be CBC (no " ") as per the canonical table.

lawrence-forooghian · 2022-07-05T20:24:28Z

Source/ARTCrypto.h

@@ -44,11 +51,30 @@ NS_ASSUME_NONNULL_BEGIN

 @end

+/**
+ The ARTCrypto object ensures that message payloads are encrypted, can never be decrypted by Ably, and can only be decrypted by other clients that share the same secret symmetric key.


Suggested change

The ARTCrypto object ensures that message payloads are encrypted, can never be decrypted by Ably, and can only be decrypted by other clients that share the same secret symmetric key.

The `ARTCrypto` object ensures that message payloads are encrypted, can never be decrypted by Ably, and can only be decrypted by other clients that share the same secret symmetric key.

@tbedford @m-hulbert as I mentioned elsewhere in this PR, I think this sort of formatting should be part of the canonical docs?

Agree with the change and this is also part of the canonical table already.

lawrence-forooghian · 2022-07-05T20:24:40Z

Source/ARTCrypto.h

@@ -44,11 +51,30 @@ NS_ASSUME_NONNULL_BEGIN

 @end

+/**
+ The ARTCrypto object ensures that message payloads are encrypted, can never be decrypted by Ably, and can only be decrypted by other clients that share the same secret symmetric key.


Suggested change

The ARTCrypto object ensures that message payloads are encrypted, can never be decrypted by Ably, and can only be decrypted by other clients that share the same secret symmetric key.

The ARTCrypto class ensures that message payloads are encrypted, can never be decrypted by Ably, and can only be decrypted by other clients that share the same secret symmetric key.

I'm not sure whether I agree with this description entirely. I think that the description of how encryption works in Ably is a good one, but it's not the ARTCrypto class itself that's responsible for performing the encryption. It’s more of a utility class, which helps you to create the parameters used to configure encryption elsewhere in the SDK.

Is something like the following more accurate?

The `ARTCrypto` class enables you to set the parameters required for configuring the encryption of message payloads.

How about

The ARTCrypto class enables you to generate the parameters required for configuring the encryption of message payloads.

@lawrence-forooghian @m-hulbert

lawrence-forooghian · 2022-07-05T20:33:36Z

Source/ARTCrypto.h

+/**
+ Retrieves, or optionally sets, the `ARTCipherParams` for the channel.
+ @param params Overrides the default parameters. A suitable key must be provided as a minimum.
+ @return `ARTCipherParams` A ARTCipherParams object, using the default values for any field not supplied.


Suggested change

@return `ARTCipherParams` A ARTCipherParams object, using the default values for any field not supplied.

@return `ARTCipherParams` A `ARTCipherParams` object, using the default values for any field not supplied.

lawrence-forooghian · 2022-07-05T20:34:31Z

Source/ARTCrypto.h

+/**
+ Retrieves, or optionally sets, the `ARTCipherParams` for the channel.
+ @param params Overrides the default parameters. A suitable key must be provided as a minimum.
+ @return `ARTCipherParams` A ARTCipherParams object, using the default values for any field not supplied.


I think it should be "An ARTCipherParams object", not "A".

lawrence-forooghian · 2022-07-05T20:37:39Z

Source/ARTCrypto.h

+
+/**
+ Generates a random key to be used in the encryption of the channel.
+ @param keyLength The length of the key, in bits, to be generated. If not specified, this is equal to the default keyLength of the default algorithm: for AES this is 256 bits.


Suggested change

@param keyLength The length of the key, in bits, to be generated. If not specified, this is equal to the default keyLength of the default algorithm: for AES this is 256 bits.

@param keyLength The length of the key, in bits, to be generated. If not specified, this is equal to the default key length of the default algorithm: for AES this is 256 bits.

@tbedford @m-hulbert I think an encryption algorithm is an abstract concept and not a class in our SDK, so I'm not sure that using keyLength, which suggests a property name, is appropriate here. Not sure though.

@QuintinWillison are you able to confirm this if possible?

lawrence-forooghian · 2022-07-05T20:38:03Z

Source/ARTCrypto.h

+
+/**
+ Generates a random key to be used in the encryption of the channel.
+ @param keyLength The length of the key, in bits, to be generated. If not specified, this is equal to the default keyLength of the default algorithm: for AES this is 256 bits.


@maratal what does it mean to "not specify" a keyLength here?

lawrence-forooghian · 2022-07-05T20:45:32Z

Source/ARTCrypto.h

+/**
+ Retrieves, or optionally sets, the `ARTCipherParams` for the channel.
+ @param params Overrides the default parameters. A suitable key must be provided as a minimum.
+ @return `ARTCipherParams` A ARTCipherParams object, using the default values for any field not supplied.


@maratal this question applies to all of the @return tags where you've written the return type as part of the tag – do you think this is necessary? The return type is already displayed in the Jazzy documentation as part of the method signature:

If you look at Apple's documentation (example), the "Return Value" section doesn't repeat the return type; rather, it's used to provide semantic information about the return value:

m-hulbert · 2022-07-06T14:02:59Z

Source/ARTCrypto.h

+ (NSData *)generateRandomKey:(NSUInteger)keyLength;
+
+/**
+ Same as `+getDefaultParams:`, but with the default algorithm and key length: AES, 256 bits.


The description for generateRandomKey is the following in the canonical table:

Generates a random key to be used in the encryption of the channel.

I'm assuming this should be something like "Generates a random key to use for encrypting message payloads." instead given @lawrence-forooghian 's comment on getDefaultParams.

But we have two methods with this name: one with parameter keyLength, and one without parameters. Canonical table contains only one description and notes that "If not specified, this is equal to the default keyLength of the default algorithm: for AES this is 256 bits.". So I decided to produce different description for method without parameter. Also I've messed up with the referred method: +getDefaultParams: instead of generateRandomKey:. Duh! @lawrence-forooghian @m-hulbert

maratal · 2022-09-14T11:16:43Z

Closed in favor of #1491

maratal added 2 commits June 30, 2022 22:58

Added docstrings for ARTCipherParams

385ff3d

Added docstrings for ARTCrypto

d693aa1

maratal requested review from lawrence-forooghian, QuintinWillison, Srushtika and tbedford June 30, 2022 21:17

Params name fixed

fd86ccd

github-actions bot temporarily deployed to staging/pull/1439/jazzydoc June 30, 2022 22:42 Inactive

lawrence-forooghian requested a review from m-hulbert July 5, 2022 20:12

lawrence-forooghian reviewed Jul 5, 2022

View reviewed changes

m-hulbert reviewed Jul 6, 2022

View reviewed changes

maratal added 4 commits July 7, 2022 16:47

Removed "" for CBC as per the canonical table.

b87ee16

Return statement for ARTCrypto updated

3e0875b

Updated descriptions for both generateRandomKey... methods

3cce613

Removed unnecessary return type placement.

4511851

github-actions bot deployed to staging/pull/1439/jazzydoc July 7, 2022 18:40 View deployment

Added backticks for AES and CBC

0deb0a0

QuintinWillison removed their request for review July 11, 2022 12:19

maratal closed this Sep 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`ARTCipherParams` and `ARTCrypto` doc comments #1439

`ARTCipherParams` and `ARTCrypto` doc comments #1439

maratal commented Jun 30, 2022 •

edited

Loading

maratal commented Jun 30, 2022

lawrence-forooghian commented Jul 5, 2022 •

edited

Loading

lawrence-forooghian Jul 5, 2022

m-hulbert Jul 6, 2022

maratal Jul 7, 2022

lawrence-forooghian Jul 5, 2022

m-hulbert Jul 6, 2022

maratal Jul 8, 2022

lawrence-forooghian Jul 5, 2022

m-hulbert Jul 6, 2022

maratal Jul 8, 2022

lawrence-forooghian Jul 5, 2022

m-hulbert Jul 6, 2022

lawrence-forooghian Jul 5, 2022

lawrence-forooghian Jul 5, 2022

m-hulbert Jul 6, 2022

maratal Jul 7, 2022

lawrence-forooghian Jul 5, 2022

lawrence-forooghian Jul 5, 2022

maratal Jul 8, 2022

lawrence-forooghian Jul 5, 2022

m-hulbert Jul 6, 2022

lawrence-forooghian Jul 5, 2022

maratal Jul 8, 2022

lawrence-forooghian Jul 5, 2022

maratal Jul 8, 2022

m-hulbert Jul 6, 2022 •

edited

Loading

maratal Jul 7, 2022

maratal Jul 8, 2022

maratal commented Sep 14, 2022

	The ARTCipherParams object contains properties to configure encryption for a channel.
	The `ARTCipherParams` object contains properties to configure encryption for a channel.

	/// The algorithm to use for encryption. Only AES is supported.
	/// The algorithm to use for encryption. Only `"AES"` is supported.

		@property (readonly, nonatomic) NSUInteger keyLength;

		/// The cipher mode. Only "CBC" is supported.

	/// The cipher mode. Only "CBC" is supported.
	/// The cipher mode. Only `"CBC"` is supported.

	The ARTCrypto object ensures that message payloads are encrypted, can never be decrypted by Ably, and can only be decrypted by other clients that share the same secret symmetric key.
	The `ARTCrypto` object ensures that message payloads are encrypted, can never be decrypted by Ably, and can only be decrypted by other clients that share the same secret symmetric key.

	The ARTCrypto object ensures that message payloads are encrypted, can never be decrypted by Ably, and can only be decrypted by other clients that share the same secret symmetric key.
	The ARTCrypto class ensures that message payloads are encrypted, can never be decrypted by Ably, and can only be decrypted by other clients that share the same secret symmetric key.

	@return `ARTCipherParams` A ARTCipherParams object, using the default values for any field not supplied.
	@return `ARTCipherParams` A `ARTCipherParams` object, using the default values for any field not supplied.

	@param keyLength The length of the key, in bits, to be generated. If not specified, this is equal to the default keyLength of the default algorithm: for AES this is 256 bits.
	@param keyLength The length of the key, in bits, to be generated. If not specified, this is equal to the default key length of the default algorithm: for AES this is 256 bits.

ARTCipherParams and ARTCrypto doc comments #1439

ARTCipherParams and ARTCrypto doc comments #1439

Conversation

maratal commented Jun 30, 2022 • edited Loading

maratal commented Jun 30, 2022

lawrence-forooghian commented Jul 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-hulbert Jul 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maratal commented Sep 14, 2022

`ARTCipherParams` and `ARTCrypto` doc comments #1439

`ARTCipherParams` and `ARTCrypto` doc comments #1439

maratal commented Jun 30, 2022 •

edited

Loading

lawrence-forooghian commented Jul 5, 2022 •

edited

Loading

m-hulbert Jul 6, 2022 •

edited

Loading