Create cow trading package#26
Conversation
…eed up base branch sync (cowprotocol#354) * chore: switch to pnpm and update Node.js to v20 * chore: reorganize project structure under packages/cow-sdk * feat: init monorepo structure with Turborepo * chore: format codebase with pnpm script * chore: eslint config. * chore: update lint config, .npmrc, and VSCode settings * feat: copy entire src, tests folders, and package/tsconfig files from app-data project into the monorepo * feat: add typescript-config * feat: create common package with abstract adapter and context classes * feat: implement adapters for ethers v5, ethers v6, and viem * chore: update pnpm-lock * chore: remove appdata and cow-sdk from build * feat: create config package * chore: fix eslint tsconfig path * feat: refactor config * chore: update pnpm-lock * feat: add contracts-ts package * feat: refactor contracts-ts * feat: update adapters with new contracts-ts features * feat: add tests for contracts-ts * feat: move order-signing to new package * feat: refactor order-signing * feat: add new order-signing features to adapters * feat: refactor order-signing tests * feat: implement adapter usage in app-data; update app-data config and test files * chore: update pnpm-lock file * chore: fix lint and remove cow-sdk from lint. * chore: update sdk-config tsconfig * feat: add contracts-ts package * feat: refactor contracts-ts * feat: update adapters with new contracts-ts features * feat: add tests for contracts-ts * chore: update contracts-ts config * chore(order-book): move order-book to monorepo package * restore: bring back deleted type files with their history * restore: bring back deleted config and path files with their history * refactor: move cow-error and wallets.ts and remove duplicate types * feat(order-book): setup order-book package, jest and tsconfig * feat(order-book): refact order-book * feat(monorepo-config): adjust all package.json and scripts * chore(lint): lint * chore(subgraph): move subgraph to monorepo package * chore(subgraph): create package, tsconfig and jest config * feat(subgraph): refactor subgraph package * chore(subgraph): update pnpm-lock and run lint * chore: add hashDomain to abstract utils * feat: move composable package from original sdk to refactor sdk * chore(subgraph): move cow-shed to monorepo package * chore(cow-shed): copy wallets.ts * feat(cow-shed): create package.json and tsconfig.json * feat(cow-shed): add estimateGas method to SignerAdapters * feat: refactor composable module to be framework-agnostic * feat: enhance adapters and utils with composable features * chore: move composable tests file to composable/tests * feat: update tests to work with ethersV5 adapter * feat: enhance composable to test the 3 adapters * chore: remove console logs * feat(cow-shed): refact on common and contract-ts * chore(cow-shed): rename CowShedHooks test file * feat(cow-shed): refact cow-she to use adapters and fix tests * fix(cow-shed): fix adapters - estimateGas and encodeAbi * chore: lint files * chore: move constants to sdk-common * feat: update Node.js to version 22 * chore: align settings.json with .editorconfig configuration. * chore: adjust cow-shed hooks test * chore: add check to verify if the signature is the same across all three adapters in cowshedHooks test * chore: apply PR suggestions * chore: update pnpm-lock.json * fix: viemUtils encodeAbi --------- Co-authored-by: Jean Neiverth <jeanneiverth@gmail.com> Co-authored-by: Jean Neiverth <79885562+jean-neiverth@users.noreply.github.com>
…, update tests, and add package, tsconfig and jest.config files
… with generic interfaces
|
I have read the CLA Document and I hereby sign the CLA 0 out of 2 committers have signed the CLA. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub. |
… for signer creation and update trading package to use the new adapter interface
jeffersonBastos
changed the title
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
jean-neiverth
left a comment
jean-neiverth
left a comment
There was a problem hiding this comment.
Took a look on the changes in composable, contracts-ts and order signing. They make sense to me, great job!
| @@ -2,13 +2,15 @@ | |||
| "extends": "@repo/typescript-config/base.json", | |||
There was a problem hiding this comment.
❌ #mustset a base config, not sure why this hasn't errored out before
There was a problem hiding this comment.
I didn’t get the issue... What exactly was supposed to throw an error?
Is it because it was overwriting the settings and should’ve thrown an error?
If so, it doesn’t actually throw an error—it allows overwriting the configurations, which is the expected behavior.
But I’ve already removed the specific settings from app-data.
There was a problem hiding this comment.
the issue here is that there's no @reporight? So there's nowhere for TS to look for when you reference @repo.
There was a problem hiding this comment.
I investigated the @repo reference issue, and here's what I found:
The @repo reference is working correctly because:
1 - The package has "@repo/typescript-config": "workspace:*" in its devDependencies
2 - PNPM workspace creates the necessary symlinks for TypeScript to resolve @repo
3 - The base config file exists at packages/typescript-config/base.json
4 - When I ran npx tsc --showConfig, it successfully loaded and merged the base configuration
Why it hasn't errored: TypeScript can resolve @repo through normal Node.js module resolution thanks to the workspace dependency. The workspace:* syntax tells pnpm to create a symlink, making @repo/typescript-config available in node_modules.
However, you're right that this could be more explicit. We could add a root tsconfig.json with proper path mapping for better clarity and IDE support. Would you like me to set that up?
| setGlobalAdapter(adapters.viemAdapter) | ||
| const viemApi = new Api('mainnet', Environment.Prod) | ||
| const viemResult = await ethersV6Api.estimateTradeAmount(query) | ||
| const viemResult = await viemApi.estimateTradeAmount(query) |
There was a problem hiding this comment.
same as https://github.com/bleu/cow-sdk/pull/26/files#r2160984573. Also if this is an e2e test it should be marked as such.
|
|
||
| // Create an encoder with incomplete price data | ||
| setGlobalAdapter(adapters[adapterNames[0]!]) | ||
| setGlobalAdapter(adapters.ethersV5Adapter) |
There was a problem hiding this comment.
ribeirojose
left a comment
ribeirojose
left a comment
There was a problem hiding this comment.
another thing I noticed is that sometimes we're using a structure for packages with tests within the source folder (e.g. where pkg/src/**/*.ts and pkg/src/**/*.(test|spec).ts live all under the source folder. And sometimes we put tests in their own tests folder under on the same level of the src folder. I assume there's a specific reason for that, could you help me understand this?
| '0x0000000000000000000000000000000000000000', | ||
| this.provider, | ||
| ) as ethers.Signer & TypedDataSigner | ||
| // No signer provided, create VoidSigner for read-only operations |
There was a problem hiding this comment.
🤔 interesting pattern. Wouldn't read operations know that they don't need a signer?
There was a problem hiding this comment.
What do you mean? Read operations will always work—I believe the issue would be with write operations.
Are you asking if those operations need to be aware that it’s a void signer?
There was a problem hiding this comment.
what I mean is should we even need a void signer if say we're only doing read operations?
There was a problem hiding this comment.
I think I get it now—I talked to Pedro to better understand this need. It’s true that there’s no need to create a void signer as the default; the client using the SDK can create a void signer if needed.
I’ll make some changes and let you know once they’re ready.
Thanks!
There was a problem hiding this comment.
I updated the adapter by removing the default creation of the void signer.
Could you review it again? TKS!
There was a problem hiding this comment.
Huh, great job refactoring lots of stuff! I think I could review the most relevant parts of it and tried to leave helpful comments and suggestions. There's only #must IMO which is this comment. I highly encourage you to take a look at all the comments though. Happy to jump on a call / answer any other questions you might have wrt the review. Thanks for moving this forward!
…ust imports, move optional parameter to the end, and fix camelCase file name
jeffersonBastos
force-pushed
the
jefferson/cow-430-create-cow-trading-package
branch
from
76f1df7 to
0ac9a49
Compare
…flict with appData typings
…egate responsibility to the user
4 participants
This PR refactors the trading package.
It also refactors the adapters and the integration interface, which led to changes in other packages and their corresponding tests.