-
Notifications
You must be signed in to change notification settings - Fork 2
chore: add foundry and aqua contracts #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
feat(PT3-497): update opcodes, fix some comments
|
|
||
| test('should dock', async () => { | ||
| const aqua = new AquaProtocolContract(new Address(forkNode.addresses.aqua)); | ||
| const strategyData = { |
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.
seems like we duplicate this, maybe lets set somewhere above and use it as const STRATEGY_DATA
even better maybe have fixtures.ts file and move all the duplciates and neccessary data there, what do you think ??
even better we can have a StrategyBuilder class
for exaple withMaker, withFee, withSalt etc.
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.
It's not const as we need maker address which is available only inside test
I am not sure that adding more abstraction to tests is good idea, will move it to level to the forkEvm definition
| // Setup evm fork with escrow factory contract and users with funds | ||
| // maker have WETH | ||
| // taker have USDC on resolver contract | ||
| export async function setupEvm(config: EvmNodeConfig): Promise<ReadyEvmFork> { |
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 am thinking of adding a class based implementation here what do you think ? it will be more elegant
kind of
env = new TestEnvironment()
forkNode = await env.setup({ chainId: 1 })
btw not sure, but do not we need cleanup for containers here or no ?
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.
For now we only setup once for test file, setup for each test is too expensive, so no cleanup is needed. But agree, worth to refactor
| // maker have WETH | ||
| // taker have USDC on resolver contract | ||
| export async function setupEvm(config: EvmNodeConfig): Promise<ReadyEvmFork> { | ||
| const chainId = config.chainId || 1 |
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.
hmm, maybe lets move CONFIG in separate file, like config configuration file
* fix(PT3-497): structure * chore: contracts * feat(PT3-497): rename shared to sdk-core, add some comments on funcitons which throw an error * fic: comment * feat(PT3-497): debug opcodes (#7) * feat(PT3-497): debug opcodes * feat: debug opcodes * feat(PT3-497): opcodes debug * feat(PT3-497): Debug opcodes in program builders * chore: eslint * fix: comment * feat: architecture change * fix: remappings * chore * Update swap-vm.spec.ts --------- Co-authored-by: Vladimir Borovik <[email protected]>
Change Summary
What does this PR change?
Adds e2e tests for aqua on forked web3 node
Related Issue/Ticket:
https://1inch.atlassian.net/browse/PT3-490
Testing & Verification
How was this tested?
Risk Assessment
Risk Level:
Risks & Impact