-
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
Add simulation and token holder endpoints #88
Add simulation and token holder endpoints #88
Conversation
apps/api/src/app/routes/__chainId/tokens/__tokenAddress/usdPrice.ts
Outdated
Show resolved
Hide resolved
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.
Very nice 👍
@yvesfracari can you improve the PR description and add test steps, how to check whether the former and new APIs still work and so on. |
4d7f95a
to
e7d5711
Compare
6a95691
to
32c3ff2
Compare
I added more information on the readme, let me know if you need more information 😄 |
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.
Looks great, local tests succeeded too.
Some minor nitpick comments
} | ||
|
||
// types were found in Uniswap repository | ||
// https://github.com/Uniswap/governance-seatbelt/blob/e2c6a0b11d1660f3bd934dab0d9df3ca6f90a1a0/types.d.ts#L123 |
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 forgot to comment on this before.
Can we reach out to Tenderly as ask whether they expose those types or a way to generate ourselves rather than copying over from Uniswap's repo?
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 think we could but I would add it as a TODO to avoid blocking the PR merge. Btw, I tried to generate these types but I couldn't succeed, the gen was falling (https://docs.tenderly.co/api/openapi.json)
})); | ||
} | ||
|
||
checkBundleSimulationError( |
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.
Nitpick: make explicit private
method?
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 thought about this but prefer to make it public to test easier 😅
Context: cowprotocol/cowswap#4943
This PR:
To test:
yarn test
libs/repositories/src/SimulationRepository/SimulationrepositoryTenderly.test.ts