Update block-ref to not make duplicate request #151
Conversation
The goal of the `block-ref` middleware is to look for instances in request parameters where "latest" is being used as a block parameter and replace it with the actual latest block number (as reported by the block tracker). The middleware will then make a child request through the provider using the resolved parameters and copy the child response onto the final response object, which will be carried to the next middleware in the stack. However, this behavior is unnecessary, and in fact it makes this middleware useless, because in practice, the next middleware will end up making a request on its own, overwriting the child response gathered in this middleware. This commit changes the `block-ref` middleware so that it will apply the "latest" replacement mentioned above by changing the request parameters in place before proceeding to the next middleware. This should retain the existing behavior but avoid making an extra request. In addition, this fills in missing tests for the middleware by reusing some test helpers from a previous PR.
src/block-ref.ts
Outdated
| res.result = childRes.result; | ||
| res.error = childRes.error; | ||
|
|
||
| req.params[blockRefIndex] = latestBlock; |
There was a problem hiding this comment.
It looks like you've changed how this works by rewriting the original request, rather than sending a child request.
If that's what we want to do, that's what the block-ref-rewrite middleware already does. So I'm not sure we want to make block-ref work like that too. Not unless we're confident we would only need one, and we want to consolidate them both.
I'm not entirely sure why the two cases were split like that, or why we moved to using this strategy.
There was a problem hiding this comment.
Sigh... that's very strange...
Okay, I've reverted the behavior and fixed up the tests accordingly: 737f54e
| it('makes a direct request through the provider, replacing the block param with the latest block number', async () => { | ||
| await withTestSetup( | ||
| { | ||
| configureMiddleware: ({ provider, blockTracker }) => { |
There was a problem hiding this comment.
This is a slightly different style compared with the retryOnEmpty tests I wrote earlier. I wanted to make it so that:
- It's obvious what the middleware-under-test is. Previously this was automatically added in a helper function, now it happens within each test explicitly. Yet, it's drawn out so it's easier to see.
- If you don't need to supply a final middleware (because you're not testing that the middleware-under-test is calling the next middleware), you don't need to supply an extra option like
withFinalMiddleware: false. At the same time, a default middleware is still pushed onto the engine. The purpose of this default middleware is to serve as a reminder for the developer in case the middleware-under-test attempts to call the next middleware and that was unexpected. - If you do need to supply a final middleware because you want to test that it gets called, then you can still do that easily.
| * @param callback - A function. | ||
| * @returns Whatever the function returns. | ||
| */ | ||
| async function withTestSetup<T>( |
There was a problem hiding this comment.
A lot of the helper functions here were copied from the tests for retryOnEmpty. I plan on introducing another PR after this one which will extract these helpers from the two files.
The goal of the
block-refmiddleware is to look for instances in request parameters where "latest" is being used as a block parameter and replace it with the actual latest block number (as reported by the block tracker). The middleware will then make a child request through the provider using the resolved parameters and copy the child response onto the final response object.That is fine; however, this middleware then calls the next middleware in the stack, which in practice will make another request, and that will end up overwriting the child response set in this middleware. That makes this middleware ineffective.
So, this commit removes the
next()call from theblock-refmiddleware so that when it takes effect it stops the chain and prevents an extra request.In addition, this fills in missing tests for the middleware by reusing some test helpers from a previous PR.
Fixes #142.