-
Notifications
You must be signed in to change notification settings - Fork 7
Test: supply with native and erc20 #3
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
Conversation
| describe('When supplying assets in an available market', () => { | ||
| // Hardcoded market info for now | ||
| const marketInfo = { | ||
| address: '0x87870Bca3F3fD6335C3F4ce8392D69350B4fA4E2', |
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.
Let's move this to a test-utils const.
And let's query the reserve:
const result = await reserve(client, {
chainId: ETHEREUM_FORK_ID,
market: <new const name>,
underlyingToken: WETH_ADDRESS
})
| if (supplyInfo.value[0]?.balance.amount.value === bigDecimal('0')) { | ||
| throw new Error('User does not have supply to withdraw'); | ||
| } |
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 not an expect(...)?
| "new:package": "NODE_OPTIONS='--import tsx' plop --plopfile=plopfile.ts", | ||
| "prepublish": "pnpm run build", | ||
| "test:client": "vitest --project client", | ||
| "test:client:local": "ENVIRONMENT=local vitest --project client", |
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 is doable with a simple ENVIRONMENT=local in front of any of the commands. I am not sure if we should have these scripts proliferating.
| import { supply, withdraw } from './transactions'; | ||
| import { userSupplies } from './user'; | ||
|
|
||
| const supplyAndGetUserSupplies = async ( |
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 is a good idea.
To make these 2 helpers self contained I suggest we make the signature self-contained so it's not possible to user them wrong.
I can explain quick in an huddle.
nit: use traditional function declaration which are named and provide better insights in stacktraces.
| expect.objectContaining({ | ||
| balance: expect.objectContaining({ | ||
| amount: expect.objectContaining({ | ||
| value: expect.any(String), |
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.
Did you try expect.closeTo here so we don't need the manual handling after?
packages/client/src/test-utils.ts
Outdated
| ); | ||
| } | ||
|
|
||
| export async function getReserveInfo( |
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.
ReserveInfo is a very specific type. This function returns a Reserve, the naming could be misleading.
I suggest:
fetchReserve
The second argument seems not used, let's keep interface surfaces to a minimal until we need to add more. Less is better.
vitest.setup.ts
Outdated
| expect.extend(matchers); | ||
|
|
||
| expect.extend({ | ||
| toBeNumericStringCloseTo(received: string, expected: number, precision = 2) { |
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.
| toBeNumericStringCloseTo(received: string, expected: number, precision = 2) { | |
| toBeBigDecimalCloseTo(received: string, expected: number | string, precision = 2) { |
No description provided.