-
Notifications
You must be signed in to change notification settings - Fork 4
chore: solution brainstroming for ingres_expiry #503
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
8ae1375
5241ae9
7868bfb
8bcd5b3
ca76743
528bd16
301875d
6ee0427
44c7d67
376fa28
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,11 @@ | ||
| import { | ||
| Certificate, | ||
| Expiry, | ||
| HttpAgent, | ||
| Nonce, | ||
| defaultStrategy, | ||
| lookupResultToBuffer, | ||
| makeExpiryTransform, | ||
| makeNonceTransform, | ||
| pollForResponse as pollForResponseAgent, | ||
| type CallRequest, | ||
|
|
@@ -13,7 +15,10 @@ import { | |
| import {bufFromBufLike} from '@dfinity/candid'; | ||
| import {Principal} from '@dfinity/principal'; | ||
| import {base64ToUint8Array, isNullish, nonNullish} from '@dfinity/utils'; | ||
| import {DEFAULT_EXPIRY_DURATION} from '../constants/http-agent.constants'; | ||
| import type {IcrcCallCanisterRequestParams} from '../types/icrc-requests'; | ||
| import {generateHash} from '../utils/crypto.utils'; | ||
| import {expiryToMs, isTimestampExpired} from '../utils/custom-http-agent.utils'; | ||
|
|
||
| export type CustomHttpAgentResponse = Pick<Required<SubmitResponse>, 'requestDetails'> & { | ||
| certificate: Certificate; | ||
|
|
@@ -30,9 +35,11 @@ export class UndefinedRootKeyError extends Error {} | |
| // Therefore, it is cleaner in my opinion to encapsulate the agent rather than extend it. | ||
| export class CustomHttpAgent { | ||
| readonly #agent: HttpAgent; | ||
| #cache: Map<string, Expiry>; | ||
|
|
||
| private constructor(agent: HttpAgent) { | ||
| this.#agent = agent; | ||
| this.#cache = new Map(); | ||
| } | ||
|
|
||
| static async create( | ||
|
|
@@ -55,9 +62,14 @@ export class CustomHttpAgent { | |
| arg, | ||
| canisterId, | ||
| method: methodName, | ||
| nonce | ||
| }: Omit<IcrcCallCanisterRequestParams, 'sender'>): Promise<CustomHttpAgentResponse> => { | ||
| nonce, | ||
| sender | ||
| }: IcrcCallCanisterRequestParams): Promise<CustomHttpAgentResponse> => { | ||
| const hash = await generateHash({canisterId, sender, method: methodName, arg, nonce}); | ||
|
roman-nazaruk marked this conversation as resolved.
Outdated
|
||
| const ingressExpiry = this.getIngressExpiry(hash); | ||
|
roman-nazaruk marked this conversation as resolved.
Outdated
|
||
|
|
||
| this.attachRequestNonce({nonce}); | ||
| this.attachAddTransformExpiry(ingressExpiry); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we don't have to set a cached expiry, I would let agent-js set the expiry because they're doing some time sync vodoo.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we always need to set a transform because we cache the agent. If we don’t, we might make a call with the expiry of a previous call. Unless agent-js provides a way to remove a transformer or we re-create an instance—but I guess the first option doesn’t exist, and the second is less performant and also complicates the code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, good point. With the same argument, the current implementation of
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We recently merged a PR to overcome this issue with the approach I mentioned (#502). Not that I like it—just sharing the information to let you know it should be fine for now.
I like the idea of implementing our custom version of HttpAgentRequestTransformFn. This way, both nonce and expiry handling can be scoped within the same function and module, improving maintainability and testing. As for appending the nonce and its length to the method name—why not? However, if we can find a way to handle this in a less hacky manner (😉), not again, but fundamentally speaking sure yes, I think having one custom function is a good idea. |
||
|
|
||
| const {requestDetails, ...restResponse} = await this.#agent.call(canisterId, { | ||
| methodName, | ||
|
|
@@ -210,4 +222,21 @@ export class CustomHttpAgent { | |
| makeNonceTransform((): Nonce => base64ToUint8Array(nonce) as Nonce) | ||
| ); | ||
| } | ||
| private attachAddTransformExpiry(expiry: number): void { | ||
| this.#agent.addTransform('update', makeExpiryTransform(expiry)); | ||
| } | ||
|
|
||
| private getIngressExpiry(hash: string): number { | ||
| const existingExpiry = this.#cache.get(hash); | ||
|
|
||
| if (existingExpiry && !isTimestampExpired(expiryToMs(existingExpiry))) { | ||
|
roman-nazaruk marked this conversation as resolved.
Outdated
|
||
| const delta = expiryToMs(existingExpiry) - Date.now(); | ||
| return delta; | ||
|
roman-nazaruk marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| const newExpiry = new Expiry(DEFAULT_EXPIRY_DURATION); | ||
| this.#cache.set(hash, newExpiry); | ||
|
|
||
| return DEFAULT_EXPIRY_DURATION; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| export const DEFAULT_EXPIRY_DURATION = 5 * 60 * 1000; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| import { IcrcCallCanisterRequestParams } from 'src/types/icrc-requests'; | ||
|
|
||
| export async function generateHash(params: IcrcCallCanisterRequestParams): Promise<string> { | ||
| const jsonString = JSON.stringify(params, Object.keys(params).sort()); | ||
|
|
||
| const dataBuffer = new TextEncoder().encode(jsonString); | ||
| const hashBuffer = await crypto.subtle.digest('SHA-256', dataBuffer); | ||
|
|
||
| return bufferToHex(hashBuffer); | ||
| } | ||
|
|
||
| function bufferToHex(buffer: ArrayBuffer): string { | ||
| return [...new Uint8Array(buffer)] | ||
|
roman-nazaruk marked this conversation as resolved.
Outdated
|
||
| .map(byte => byte.toString(16).padStart(2, '0')) | ||
| .join(''); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| import {Expiry} from '@dfinity/agent'; | ||
|
|
||
| export function isTimestampExpired(timestamp: number): boolean { | ||
| return Date.now() > timestamp; | ||
| } | ||
|
|
||
| export function expiryToMs(expiry: Expiry): number { | ||
| return Number(expiry['_value'] / 1000000n); | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.