-
Notifications
You must be signed in to change notification settings - Fork 1
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
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