chore: solution brainstroming for ingres_expiry - #503
Conversation
| // TODO: Should we try with a new expiry or reject the request if the timestamp is expired? | ||
| return (!existingExpiry || expiryToMs(existingExpiry) <= Date.now()) | ||
| ? this.createAndStoreNewExpiry(hash) | ||
| : expiryToMs(existingExpiry) - Date.now(); |
There was a problem hiding this comment.
This is dangerous, because there is no guarantee that makeExpiryTransform will set the same expiry (it will call Date.now() again to convert back from delta to expiry, which might result in a slightly different value). We should implement our own HttpAgentRequestTransformFn that will directly receive an Expiry (instead of a delta) and sets it.
| 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.
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?
There was a problem hiding this comment.
I also rather like rejection. It's also easier/cleaner to implement.
| const ingressExpiry = hash ? this.getIngressExpiry(hash) : DEFAULT_EXPIRY_DURATION; | ||
|
|
||
| this.attachRequestNonce({nonce}); | ||
| this.attachAddTransformExpiry(ingressExpiry); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
I see, good point. With the same argument, the current implementation of attachRequestNonce() also shouldn't work because it conditionally sets the transform. WDYT of the following "creative" approach:
- We write a custom implementation of HttpAgentRequestTransformFn that takes care of setting the nonce and the expiry and manages the expiry cache. (Behavior described in step 4).
- In
CustomHttpAgent.constructor(agent)we set it viaagent.addTransform() - In
CustomHttpAgent.request()we append the nonce and the length of it to the method name. If no nonce is specified, we just append length 0. - In our custom transform implementation, we remove the nonce and length from the method name.
- If nonce length is 0, we're done.
- Otherwise we set the chosen nonce in the request, extract the other request parameters necessary to construct the cache key and check the expiry cache.
- On cache miss we add the expiry from the request to the cache.
- On cache hit we replace the expiry in the request with the one from the cache.
There was a problem hiding this comment.
the current implementation of attachRequestNonce() also shouldn't work
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.
following "creative" approach
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.
tmu0
left a comment
tmu0
left a comment
There was a problem hiding this comment.
general approach LGTM, only found two minor issues
| this.#cache.set(hash, requestDetails.ingress_expiry); | ||
| } | ||
|
|
||
| this.cleanCacheAfterCall(); |
There was a problem hiding this comment.
I think we agreed on not removing entries from the cache, so this method can be deleted.
| 'update', | ||
| makeNonceTransform((): Nonce => base64ToUint8Array(nonce) as Nonce) | ||
| ); | ||
| private splitModifiedMethodName(modifiedMethodName: string) { |
There was a problem hiding this comment.
this is not a safe way of extracting the nonce and hash if the original method name contains _nonce_ or _hash_
bf2858e to
6ee0427
Compare
3 participants
Motivation
Changes
Tests