generate js client with codama and add a basic test to confirm works - #73
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
581bcfa to
55f0562
Compare
…ing-libraries-for
using newer version of litesvm nodejs lib, and settlement seed now has to be imported dynamically from the IDL because of version bumping
Co-authored-by: Federico Giacon <58218759+fedgiac@users.noreply.github.com>
Co-authored-by: Federico Giacon <58218759+fedgiac@users.noreply.github.com>
* add comments for settlement instruction and validate match * simplify superfluous comments in the IDL in general * switch to using `LazyLock` and update call sites
…ies-for' of github.com:cowprotocol/solana-programs into kaze/sc-255-write-idl-and-generate-corresponding-libraries-for
Co-authored-by: Federico Giacon <58218759+fedgiac@users.noreply.github.com>
…ies-for' of github.com:cowprotocol/solana-programs into kaze/sc-255-write-idl-and-generate-corresponding-libraries-for
fedgiac
left a comment
fedgiac
left a comment
There was a problem hiding this comment.
Nice approach and the code is quite simple for what it does! A few comments but no changes to the design, which I like.
| "peerDependencies": { | ||
| "@solana/kit": "^6.10.0" | ||
| }, |
There was a problem hiding this comment.
Let me explain the pitfalls with peer dependencies with an example. I created two packages (here) that try to import our package: consumer-kit-v8 and consumer-kit-v6, which differ on the version of @solana/kit that they use (resp. 6 and 8). The actual package code is extremely trivial, and this is the point.
If you try to use them (corepack pnpm install && corepack pnpm build):
- v6 (the one we are requiring): install works; build fails because of mismatched types.
- v8 (the one hinted at by our dev dependency): install warns you that the peer dependencies are broken; build succeeds.
Now, you could argue "this problem is just because peer and dev deps don't match, the fix is easy!".
My counterargument would be that it's easy to get things wrong.
In fact, your first commit made the peer generic * (which by the way has its own set of problems); this would have made v8 work, at least. However this was changed back in 5434c20 and I suspect this was an automated change.
Now, this is not really something special of this PR. The example in the Codama readme has the same peer dependency.
The expectations that Codana puts on us are clear: you must update often. You must stay in sync with @solana/kit and release a new version as soon as possible. People who use our package and don't run up the same update treadmill won't be able to use the same features. This update philosophy is why we suffer so much on updates like in #100.
A dependency makes this much softer (internal functions still work, the only possible mismatch should be the output type). This doesn't mean that mismatching versions will just work, but the breakage is limited to the external interface of our package, not the internal functions it uses.
The reason I feel so strongly is that I had we did the same for the contracts and so, so many people were struggling installing our package because they used Ethers v6 and we're still at Ethers v5. With a dependency they could have sorted out the issue much more easily compared to a peer dependency, where instead they need to understand both the problem as well as our internals.
Look at what error appears when building v6 from above! It's deep inside the imported package and there's no clue as to what's actually going wrong.
$ corepack pnpm build
$ tsc --noEmit
node_modules/.pnpm/cow-solana-settlement-client@file+..+programs+settlement+idl+client+js_@solana+kit@6.10_69cabcb7e9f65ac7453c39c1f587e61a/node_modules/cow-solana-settlement-client/src/generated/programs/cowSettlement.ts:406:47 - error TS2345: Argument of type 'T' is not assignable to parameter of type 'ClientWithRpc<GetAccountInfoApi & GetMultipleAccountsApi>'.
Type 'CowSettlementPluginRequirements' is not assignable to type 'ClientWithRpc<GetAccountInfoApi & GetMultipleAccountsApi>'.
The types of 'rpc.getAccountInfo' are incompatible between these types.
Type '((address: import("/mnt/lima-solana/repos/solana-programs/solana-program-workbench/consumer-kit-v6/node_modules/.pnpm/@solana+addresses@6.10.0_typescript@5.9.3/node_modules/@solana/addresses/dist/types/address").Address, config: Readonly<...> & ... 1 more ... & Readonly<...>) => import("/mnt/lima-solana/repos/solana...' is not assignable to type '((address: import("/mnt/lima-solana/repos/solana-programs/solana-program-workbench/consumer-kit-v6/node_modules/.pnpm/@solana+addresses@8.2.0_typescript@5.9.3/node_modules/@solana/addresses/dist/types/address").Address, config: Readonly<...> & ... 1 more ... & Readonly<...>) => import("/mnt/lima-solana/repos/solana-...'.
Type '((address: Address, config: Readonly<{ commitment?: Commitment | undefined; encoding: "base58" | "base64" | "base64+zstd" | "jsonParsed"; minContextSlot?: bigint | undefined; }> & Readonly<...> & Readonly<...>) => PendingRpcRequest<...>) & ... 29 more ... & ((address: Address, config?: Omit<...> | undefined) => Pend...' is not assignable to type '(address: Address, config: Readonly<{ commitment?: Commitment | undefined; encoding: "base58" | "base64" | "base64+zstd" | "jsonParsed"; minContextSlot?: bigint | undefined; }> & Readonly<...> & Readonly<...>) => PendingRpcRequest<...>'.
Call signature return types 'PendingRpcRequest<Readonly<{ context: Readonly<{ slot: bigint; }>; value: GetAccountInfoApiResponse<Readonly<{ data: Base64EncodedDataResponse; }>>; }>>' and 'PendingRpcRequest<Readonly<{ context: Readonly<{ slot: bigint; }>; value: GetAccountInfoApiResponse<Readonly<{ data: Base64EncodedDataResponse; }>>; }>>' are incompatible.
The types returned by 'reactiveStore()' are incompatible between these types.
Property 'withSignal' is missing in type 'import("/mnt/lima-solana/repos/solana-programs/solana-program-workbench/consumer-kit-v6/node_modules/.pnpm/@solana+subscribable@6.10.0_typescript@5.9.3/node_modules/@solana/subscribable/dist/types/reactive-action-store").ReactiveActionStore<[], Readonly<{ context: Readonly<{ slot: bigint; }>; value: GetAccountInfoAp...' but required in type 'import("/mnt/lima-solana/repos/solana-programs/solana-program-workbench/consumer-kit-v6/node_modules/.pnpm/@solana+subscribable@8.2.0_typescript@5.9.3/node_modules/@solana/subscribable/dist/types/reactive-action-store").ReactiveActionStore<[], Readonly<{ context: Readonly<{ slot: bigint; }>; value: GetAccountInfoApi...'.
406 orderAccount: addSelfFetchFunctions(client, getOrderAccountCodec()),
~~~~~~
node_modules/.pnpm/@solana+subscribable@8.2.0_typescript@5.9.3/node_modules/@solana/subscribable/dist/types/reactive-action-store.d.ts:81:14
81 readonly withSignal: (signal: AbortSignal) => {
~~~~~~~~~~
'withSignal' is declared here.
node_modules/.pnpm/cow-solana-settlement-client@file+..+programs+settlement+idl+client+js_@solana+kit@6.10_69cabcb7e9f65ac7453c39c1f587e61a/node_modules/cow-solana-settlement-client/src/generated/programs/cowSettlement.ts:408:13 - error TS2345: Argument of type 'T' is not assignable to parameter of type 'ClientWithRpc<GetAccountInfoApi & GetMultipleAccountsApi>'.
Type 'CowSettlementPluginRequirements' is not assignable to type 'ClientWithRpc<GetAccountInfoApi & GetMultipleAccountsApi>'.
The types of 'rpc.getAccountInfo' are incompatible between these types.
Type '((address: import("/mnt/lima-solana/repos/solana-programs/solana-program-workbench/consumer-kit-v6/node_modules/.pnpm/@solana+addresses@6.10.0_typescript@5.9.3/node_modules/@solana/addresses/dist/types/address").Address, config: Readonly<...> & ... 1 more ... & Readonly<...>) => import("/mnt/lima-solana/repos/solana...' is not assignable to type '((address: import("/mnt/lima-solana/repos/solana-programs/solana-program-workbench/consumer-kit-v6/node_modules/.pnpm/@solana+addresses@8.2.0_typescript@5.9.3/node_modules/@solana/addresses/dist/types/address").Address, config: Readonly<...> & ... 1 more ... & Readonly<...>) => import("/mnt/lima-solana/repos/solana-...'.
Type '((address: Address, config: Readonly<{ commitment?: Commitment | undefined; encoding: "base58" | "base64" | "base64+zstd" | "jsonParsed"; minContextSlot?: bigint | undefined; }> & Readonly<...> & Readonly<...>) => PendingRpcRequest<...>) & ... 29 more ... & ((address: Address, config?: Omit<...> | undefined) => Pend...' is not assignable to type '(address: Address, config: Readonly<{ commitment?: Commitment | undefined; encoding: "base58" | "base64" | "base64+zstd" | "jsonParsed"; minContextSlot?: bigint | undefined; }> & Readonly<...> & Readonly<...>) => PendingRpcRequest<...>'.
Call signature return types 'PendingRpcRequest<Readonly<{ context: Readonly<{ slot: bigint; }>; value: GetAccountInfoApiResponse<Readonly<{ data: Base64EncodedDataResponse; }>>; }>>' and 'PendingRpcRequest<Readonly<{ context: Readonly<{ slot: bigint; }>; value: GetAccountInfoApiResponse<Readonly<{ data: Base64EncodedDataResponse; }>>; }>>' are incompatible.
The types returned by 'reactiveStore()' are incompatible between these types.
Property 'withSignal' is missing in type 'import("/mnt/lima-solana/repos/solana-programs/solana-program-workbench/consumer-kit-v6/node_modules/.pnpm/@solana+subscribable@6.10.0_typescript@5.9.3/node_modules/@solana/subscribable/dist/types/reactive-action-store").ReactiveActionStore<[], Readonly<{ context: Readonly<{ slot: bigint; }>; value: GetAccountInfoAp...' but required in type 'import("/mnt/lima-solana/repos/solana-programs/solana-program-workbench/consumer-kit-v6/node_modules/.pnpm/@solana+subscribable@8.2.0_typescript@5.9.3/node_modules/@solana/subscribable/dist/types/reactive-action-store").ReactiveActionStore<[], Readonly<{ context: Readonly<{ slot: bigint; }>; value: GetAccountInfoApi...'.
408 client,
~~~~~~
node_modules/.pnpm/@solana+subscribable@8.2.0_typescript@5.9.3/node_modules/@solana/subscribable/dist/types/reactive-action-store.d.ts:81:14
81 readonly withSignal: (signal: AbortSignal) => {
~~~~~~~~~~
'withSignal' is declared here.
Found 2 errors in the same file, starting at: node_modules/.pnpm/cow-solana-settlement-client@file+..+programs+settlement+idl+client+js_@solana+kit@6.10_69cabcb7e9f65ac7453c39c1f587e61a/node_modules/cow-solana-settlement-client/src/generated/programs/cowSettlement.ts:406
[ELIFECYCLE] Command failed with exit code 2.
Note they are all optional.
What I do think we should do is widen the list of available versions to maybe ^6.x which will prevent unnecessary compatibility issues
We absolutely should. And I don't think we can do better, we aren't compatible with other versions other than 8 (not 6). So ^8. Once we do that, people will come to us and nag us to update the package because they can't use this package with a future version 9 if we don't do something about it. This is the annoying part that wouldn't be as bad with a dependency.
It actually helps reduce problems with downloading multiple package versions and ultimately ending up with either type or compatibility errors because you are unknowingly using distinct/separate library versions, especially when working with web applications.
What I think it's happening here is that the package is too strict in package.json with the versioning (something like "mydep": "^1.42.1337" when "mydep": "^1" would be fine). The version of a dependency is honored; the version of a peer isn't, it just fires a warning (this is why v8 above builds). So automatic resolution ends up using different versions of the package than what the package technically requires, this adds an install warning that's ignored, and this still works because the original specs are too strict.
That said, I'm not a web dev, this is just my frustration with using peer deps in JS. There's certainly another side of the coin, the dependency problem are a thing as well.
That said, what next? Two options, I let you pick:
- We use peer dependencies. This is actually what Codama suggests, which is why I don't push only for using normal dependencies. We use the latest version for now, strict in the dev deps (
^8.2), loose in the peers (^8). In terms of maintenance, we stick to whatever the frontend uses. If other people want different version, we don't support that. - We use dependencies, strict in the dev deps (
^8.2), loose in the peers (^8). I believe this makes it easier for people using a different version of@solana/kitto still use our package. Some function will be broken because the types may not match, but where this isn't an issue it will just work, unlike with peer dependencies. The only reason not to choose this is that Codama wants us to do things differently and I admit my knowledge could be lacking, but I'd like a concrete failure before dismissing it.
Co-authored-by: Federico Giacon <58218759+fedgiac@users.noreply.github.com>
…ding-libraries-for
fedgiac
left a comment
fedgiac
left a comment
There was a problem hiding this comment.
Looks good, last comments. Also linking open discussion from older reviews:
| "peerDependencies": { | ||
| "@solana/kit": "^6.10.0" | ||
| }, |
There was a problem hiding this comment.
Added an issue for testing on our end that the package is good, for now let's keep it as it is and rely on the frontend to tell us if there are issues. #141
…ies-for' of github.com:cowprotocol/solana-programs into kaze/sc-255-write-idl-and-generate-corresponding-libraries-for
fedgiac
left a comment
fedgiac
left a comment
There was a problem hiding this comment.
Looks good! Remaining comments are nits, feel free to skip if you're unsure.
Co-authored-by: Federico Giacon <58218759+fedgiac@users.noreply.github.com>
…ies-for' of github.com:cowprotocol/solana-programs into kaze/sc-255-write-idl-and-generate-corresponding-libraries-for
3 participants
Description
Using the IDL from #65 , generate a TS/JS client for the settlement program, and write a test to verify the critical path
createOrderinstruction is convenient and correct to use.Changes and Rationale
The JS/TS client was selected because the backend has an immediate dependency on it. A rust library can also be generated with codama which would allow for us to have E2E IDL library generation tests, but this has been left as an issue for another time. Codama is also a native Node.JS application, so javascript dependencies were going to come one way or another.
The library is generated through a script stored alongside the IDL,
generate.mjs, which invokes codama. It also includes certain overrides and customized settingsIt is important that clients are able to call
createOrderwithout having to make any unnecessary changes, but the IDL cannot fully express the grammar for thecreateOrderinstruction due to theorderPdadepending upon a computed hash. Thankfully, codama providesresolverValueNode, which allows for defining a custom function in the destination language that can be hooked in to provide this missing functionality. It requires adding ahooks.tsfile in the source directory which codama hardcodes to read.The flags for an order cannot be expressed in IDL directly either, so a helper file was added for this.
PNPM is used as the package manager. This matches the package manager used by the frontend. The correct version is locked and the Justfile commands have it installed through
corepack, a utility that is shipped with modern versions of Node.JS.The single typescript test was added to the CI as a new job.
Out of scope
This is not intended to be a comprehensive test of the IDL generation or all the instructions exported by the library. Its just to give us a starting point to make sure that the IDL generation and the corresponding library it generates works as expected.
Getting every single instruction that is not currently IDL compatible at all (such as
BeginSettleandFinalizeSettle) is a much more complicated task and also has been left out.Releasing the generated package to npm registry or elsewhere for consumption is also kept outside the scope of this PR.
How to Test/Use
Node.js is now a dependency of the repository for a full build. Node.js can be installed many ways, but I recommend NVM. I used node.js 24 for my testing and the CI.
just generate-js-clientto create the generated filesjust test-js-clientto run the one JS lib test.