-
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 4 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} 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 = nonce ? await generateHash({ canisterId, sender, method: methodName, arg, nonce }) : null; | ||
| const ingressExpiry = hash ? this.getIngressExpiry(hash) : DEFAULT_EXPIRY_DURATION; | ||
|
|
||
| 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, | ||
|
|
@@ -77,6 +89,8 @@ export class CustomHttpAgent { | |
| canisterId | ||
| }); | ||
|
|
||
| this.cleanCacheAfterCall(); | ||
|
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 agreed on not removing entries from the cache, so this method can be deleted. |
||
|
|
||
| // I assume that if we get a result at this point, it means we can respond to the caller. | ||
| // However, this is not how it's handled in Agent-js. For some reason, regardless of whether they get a result at this point or not, if the response has a status of 202, they overwrite the result with pollForResponse, which seems incorrect. | ||
| // That is why we return the result if we get one. | ||
|
|
@@ -210,4 +224,32 @@ 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); | ||
|
|
||
| // TODO: Should we try with a new expiry or reject the request if the timestamp is expired? | ||
|
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. After thinking a bit more about it, it might be better to reject the request if the timestamp is expired instead of generating a new one (and also not removing expired timestamps from the cache), WDYT?
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 also rather like rejection. It's also easier/cleaner to implement. |
||
| return (!existingExpiry || expiryToMs(existingExpiry) <= Date.now()) | ||
| ? this.createAndStoreNewExpiry(hash) | ||
| : expiryToMs(existingExpiry) - Date.now(); | ||
|
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. This is dangerous, because there is no guarantee that |
||
| } | ||
|
|
||
| private createAndStoreNewExpiry(hash: string): number { | ||
| const newExpiry = new Expiry(DEFAULT_EXPIRY_DURATION); | ||
| this.#cache.set(hash, newExpiry); | ||
| return DEFAULT_EXPIRY_DURATION; | ||
| } | ||
|
|
||
| private cleanCacheAfterCall(): void { | ||
| const now = Date.now(); | ||
|
|
||
| for (const [key, expiry] of this.#cache.entries()) { | ||
| if (expiryToMs(expiry) <= now) { | ||
| this.#cache.delete(key); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| 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,11 @@ | ||
| import {uint8ArrayToHexString} from '@dfinity/utils'; | ||
| 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 uint8ArrayToHexString(new Uint8Array(hashBuffer)); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| import {Expiry} from '@dfinity/agent'; | ||
|
|
||
| export function expiryToMs(expiry: Expiry): number { | ||
| return Number(expiry['_value'] / 1000000n); | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.