-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Implement service using IoC #50
Conversation
@@ -1,3 +1,5 @@ | |||
import 'reflect-metadata'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem to be used anywhere. Is importing enough to "activate" it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, indeed. This allows us to use the introspection to make annotations work
}); | ||
|
||
it('should return always 1234', async () => { | ||
expect(await slippageService.getSlippageBps('0x0', '0x0')).toEqual(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test description doesn't match the result
expect(await slippageService.getSlippageBps('0x0', '0x0')).toEqual(0); | |
expect(await slippageService.getSlippageBps('0x0', '0x0')).toEqual(1234); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just changed a silly implementation for another. This will change soon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyways, fixed (set to 0 in the description)
} | ||
|
||
// TODO: Find good name for the implementation, as Leandro don't like "Impl" suffix, just don't want to couple it to add Coingecko in its name (as that would couple it to the data-source, and the adding a name to specify the algorithm might complicate the name, as this will use some custom logic based on standard deviation, so for now as we plan to have just one implementation, I want to keep it simple. But happy to get ideas here) | ||
export const slippageServiceSymbol = Symbol.for('SlippageService'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the symbol? Is that a common pattern with injectable
?
Also, since it's a const, should it be all CAPS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shoom3301 @alfetopito I will merge this PR and just iterate. I will give it a try in a bit to the other framework proposed by Sasha, I think it will be easier to just iterate from the state of this PR |
Continues on #49
Integrates
inversify
, an IoC framework.It is used to replace the factory method added #48 to inject the slippage service using IoC.
I also added a unit test for the service, where I mocked the repository to be able to test the service in isolation
When reviewing this PR
I didn't care about the implementation of the repository (is mocked) or the service (I made also mocked implementation that returns random numbers).
The point is to decouple the layers, and integrate this framework.
Some future PR will make the real implementation for the repositories and services.
Test
Make sure they tests passes:
nx run services:test --watch
Make sure the service is available:
http://localhost:3010/chains/1/markets/0x6b175474e89094c44da98b954eedeac495271d0f-0x2260fac5e5542a773aa44fbcfedf7c193bc2c599/slippageTolerance
If you keep refreshing, you'll see the result keeps changing
Screen.Recording.2024-07-08.at.22.52.19.mov