Skip to content

Conversation

@ilbertt
Copy link
Contributor

@ilbertt ilbertt commented Dec 29, 2025

Rework of the http-auth-js library to use the new HTTP protocol with BHTTP.

Blocked by #31.

@ilbertt ilbertt requested a review from a team as a code owner December 29, 2025 10:17
Base automatically changed from luca/ic-http to main December 29, 2025 10:37
Copy link
Member

@peterpeterparker peterpeterparker left a comment

Choose a reason for hiding this comment

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

Not really a review, more rubber stamp with few comments that cross my mind on a quick read.

* Calculates the ingress expiry timestamp in nanoseconds.
*/
function calculateIngressExpiry(expirationTimeMs: number): bigint {
const expiryTimestampMs = Date.now() + expirationTimeMs;
Copy link
Member

Choose a reason for hiding this comment

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

in @dfinity/utils there is a nowInBigIntNanoSeconds which can be handy here to remain in nanos

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be nice to keep this package free of dependencies as much as possible. The current @icp-sdk/core package is there just for speed, but ideally we could move here all the utility functions here

}

// Set all authentication headers
req.headers.set(SIGNATURE_HEADER_NAME, signatureHeaderValue);
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: found this imperative approach difficult to decrypt, skipped it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean? Do you have a suggestion on how to improve it?

}

function getSignatureHeaderValue(signatureName: SignatureName, signature: ArrayBuffer): string {
return `${signatureName}=:${base64Encode(signature)}:`;
Copy link
Member

Choose a reason for hiding this comment

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

No separator constant for =: unlike other separators?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to keep the function as the source of truth in this case for better clarity

@ilbertt ilbertt merged commit 16bc4c5 into main Dec 29, 2025
3 checks passed
@ilbertt ilbertt deleted the luca/http-auth-draft branch December 29, 2025 11:41
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