sui cli: upstream sui-client-gen #15984
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
4 Ignored Deployments
|
|
The bigger question I'd have is how to upstream the tests https://github.com/kunalabs-io/sui-client-gen/blob/master/ts/tests/source.test.ts. These e2e tests are very important. They cover many things, including function call arg encoding for all the different types, calling transactions, fetching objects from chain and various ways of decoding, generics, special types, serialization, upgraded packages, and a bunch of type inferrence. I wouldn't touch the generator without running the tests. It's almost impossible to develop without them. So the test setup is as follows:
This test setup worked very well for me in my small repo but will probably be an issue here with CI. How do we integrate this here with your testing setup? |
sblackshear
requested review from
stefan-mysten,
tzakian,
amnn,
Jordan-Mysten and
hayes-mysten
Hmm, this sounds interesting/tricky. @Jordan-Mysten and @hayes-mysten would probably have the best ideas |
|
@sblackshear For testing, we should probably copy the localnet setup we use for our main products, where we spin up a local network + faucet, and test using that, it shouldn't be too hard to get that dialed in for this. |
|
@Jordan-Mysten in that case will you guys do it or can you give me some pointers for where this code should be located and some examples? I imagine ideally we'd have:
One problem might arise from the generated "snapshot" code commited to the repo because the package id is hard-coded in many places, so if you try to re-run the generator on a package published on local net it will generate always different ids (unless there's a way to set the published package id deterministically for the test environment?) |
|
@kklas Thanks for this great work. I promise to look more in-depth a bit later, but regarding testing, the CLI has a bunch of tests that use the |
|
Thanks @stefan-mysten. In that case I think it might be best to:
In this setup we don't have CI testing against the current version of
|
| "#; | ||
|
|
||
| pub static LOADER: &str = r#" | ||
| import { compressSuiType, parseTypeName } from './util' |
There was a problem hiding this comment.
it might be cleaner to publish all the static/shared files as a new npm package rather than generating them out. The big downside with this is versioning, since now you have a dependency between the generator and the package that has the shared definitions. It wouldn't be much different than the dependency on @mysten/sui.js that already exists, so that might be fine
There was a problem hiding this comment.
There's 2 more downsides:
- users have to install an additional dependency for the generated code (which is a small friction for adoption), and also manually keep it in sync with new versions of generated code
- there's another npm package that you have to manage
Personally I don't see any advantages to doing it via a separate package (only downsides) so that's why I did it this way, but once it's upstreamed it's up to you. My recommendation is to generate it.
|
Testing is a tricky problem for generators, especially when the outputs are inconsistent. I haven't had time to look into it too deeply, but what I would likely do would be:
|
|
@hayes-mysten so from what I understand, your setup is actually very similar to what I described above, with the difference being that the e2e tests would live in the SDK e2e directory instead of within the rust crate. I think that this could also work well, the main difference being we'd have to run the generator with
Clever, I'd have to double check but I think that would work well. IDs are hard coded in some places but they can be easily imported (there's already a separate file with IDs).
This is cool, I didn't know about it.
We have this already pretty much with the current tests. Creating an example app is probably not necessary IMO because pretty much all of it should be covered with the existing tests unless perhaps you want to test that composition of generated code in a real life app works correctly or something (but I guess you can also do that with normal tests). |
|
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
|
@kklas we still want to merge this, but sorry we dropped the ball here and thanks for your patience. |
|
@stefan-mysten yes, we definitely need to add testing. I haven't had the time to set this up yet, but we definitely want to have testing that validates both the generated code, and that the generated code runs correctly, and that type-checking on the generated code with typescript works as expected. I've been pretty focused on work to get ready for a 1.0 release of the SDK, and the GraphQL migration, but I'll see if I can find some time in the next few weeks to make some progress here |
Awseome. We definitely want this in, so looking forward to your support with testing here. |
|
Sorry it's taken me so long to get around to looking into the test set up. I'm going to try to start setting up something today |
hayes-mysten
force-pushed
the
upstream-sui-client-gen
branch
from
cbdeb51
to
f4c5743
Compare
…ed)" This reverts commit da5f0ed.
kklas
force-pushed
the
upstream-sui-client-gen
branch
from
b157da4
to
9b36ac3
Compare
|
@kklas is attempting to deploy a commit to the Mysten Labs Team on Vercel. A member of the Team first needs to authorize it. |
|
@kklas, thanks for keeping working on this! What do you think it is left to get this ready to merge? |
|
@hayes-mysten I've made the test run against localnet:
This is perhaps not a blocker here but having snapshot tests would be a good idea. |
|
What kind of snapshots are you thinking of? (need to read the code a bit, I lost a lot of the context over time). |
|
Something like this:
|
|
Semgrep found 3
Risk: Affected versions of vite are vulnerable to Improper Handling Of Case Sensitivity / Exposure Of Sensitive Information To An Unauthorized Actor / Improper Access Control. The vulnerability arises when the Vite development server's option, Manual Review Advice: A vulnerability from this advisory is reachable if you host vite's development server on Windows, and you rely on Fix: Upgrade this library to at least version 4.5.2 at sui/examples/trading/frontend/pnpm-lock.yaml:4700. Reference(s): GHSA-c24v-8rfc-w8vw, CVE-2023-34092, CVE-2024-23331 Ignore this finding from ssc-efa14576-9601-4ae6-939c-3da58aa25013.Semgrep found 2 Risk: Affected versions of @babel/traverse are vulnerable to Incomplete List Of Disallowed Inputs. An attacker can exploit a vulnerability in the internal Babel methods Manual Review Advice: A vulnerability from this advisory is reachable if you use a 3rd party plugin that relies on the Fix: Upgrade this library to at least version 7.23.2 at sui/pnpm-lock.yaml:3959. Reference(s): GHSA-67hx-6x53-jw92, CVE-2023-45133 Ignore this finding from ssc-aff5e8de-c638-4356-8a93-120597e35ce9.Semgrep found 1 Risk: Affected version of get-func-name is vulnerable to Uncontrolled Resource Consumption / Inefficient Regular Expression Complexity. The current regex implementation for parsing values in the module is susceptible to excessive backtracking, leading to potential DoS attacks. Fix: Upgrade this library to at least version 2.0.1 at sui/pnpm-lock.yaml:16906. Reference(s): GHSA-4q6p-r6v2-jvc5, CVE-2023-43646 Ignore this finding from ssc-5a557c33-4191-4714-a574-8efb44cf209b. |
Description
This upstreams
sui-client-genfrom https://github.com/kunalabs-io/sui-client-gen to thesuiCLI as a new subcommandsui client-gen.A new crate has been created (
sui-client-gen) that exposes therun_client_genmethod which is ran from thesuicrate as a clap subcommand.Test Plan
How did you test the new or updated feature?
If your changes are not user-facing and do not break anything, you can skip the following section. Otherwise, please briefly describe what has changed under the Release Notes section.
Type of Change (Check all that apply)
Release notes