Add generation of contract clients and an AssembledTransaction abstraction
#891
Conversation
chadoh
force-pushed
the
feat/contract-client-from-spec
branch
from
192290f to
2ee71ea
Compare
willemneal
force-pushed
the
feat/contract-client-from-spec
branch
from
2ee71ea to
63d6000
Compare
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: @typescript-eslint/[email protected] |
chadoh
force-pushed
the
feat/contract-client-from-spec
branch
from
0fdd8cb to
4c17f84
Compare
willemneal
force-pushed
the
feat/contract-client-from-spec
branch
from
4c17f84 to
083dabf
Compare
chadoh
force-pushed
the
feat/contract-client-from-spec
branch
from
083dabf to
64987d4
Compare
|
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
willemneal
force-pushed
the
feat/contract-client-from-spec
branch
from
d09d8ed to
16d50ba
Compare
chadoh
force-pushed
the
feat/contract-client-from-spec
branch
5 times, most recently
from
0792c12 to
b35612c
Compare
|
Blocked by quickstart not starting friendbot |
There was a problem hiding this comment.
I did an initial review pass, but soon realized I have no context on what I'm reviewing. So sorry for the dumb question, but is there a scope of work that defines the why behind this PR? I'd like to see the motivations/requirements/requests to understand it better and provide better feedback.
In general, I'm really not a fan of the SDK now having a dependency on the Soroban CLI, even if it's just for running tests. Can we replicate the functionality using the SDK itself?
Even more broadly, this work feels like it belongs at an even higher level than the SDK itself... We've moved far beyond the "provide access to endpoints + tx building + small misc. XDR wrappers". I would go so far as to say that this could even be its own repository: it feels more like a one-time tool than something that belongs in the bundle of every user who ever installs stellar-sdk.
Thoughts?
Edit: After a thorough dive into the PR description, I see the rationale: moving stuff closer to the SDK. But I'm afraid I still can't really see how this belongs here. It almost seems like a high-level wrapper layer on top of Soroban interactions, but it goes WELL beyond the scope of the SDK and makes some very opinionated abstractions that don't necessarily have a generic value-add... Especially the reliance on Freighter really feels wrong here.
| private simulation?: SorobanRpc.Api.SimulateTransactionResponse; | ||
| private simulationResult?: SorobanRpc.Api.SimulateHostFunctionResult; | ||
| private simulationTransactionData?: xdr.SorobanTransactionData; |
There was a problem hiding this comment.
Aren't the latter two already part of the first one?
There was a problem hiding this comment.
We would prefer to only have the first one, but couldn't figure out serialization/deserialization if we only had simulation. Adding the other two was a workaround. Maybe you could help us figure out simulation serialization!
| { | ||
| tx, | ||
| simulationResult, | ||
| simulationTransactionData, | ||
| }: { | ||
| tx: XDR_BASE64; | ||
| simulationResult: { | ||
| auth: XDR_BASE64[]; | ||
| retval: XDR_BASE64; | ||
| }; | ||
| simulationTransactionData: XDR_BASE64; | ||
| } |
There was a problem hiding this comment.
Could this just be Transaction and Api.SimulateTransactionSuccessResponse?
There was a problem hiding this comment.
Maybe. Again, this was working around errors we were getting while serializing/deserializing.
| } | ||
| } | ||
|
|
||
| parseError(errorMessage: string): Result<never, ErrorMessage> | undefined { |
There was a problem hiding this comment.
I would rather return errorMessage than undefined in the mismatched cases.
There was a problem hiding this comment.
The errors in Result should align with the contract's errors. So would you prefer it throws instead? Otherwise not sure which contract error to give it.
| getPublicKey = async (): Promise<string | undefined> => { | ||
| const wallet = this.options.wallet; | ||
| if (!(await wallet.isConnected()) || !(await wallet.isAllowed())) { | ||
| return undefined; | ||
| } | ||
| return (await wallet.getUserInfo()).publicKey; | ||
| }; |
There was a problem hiding this comment.
This doesn't feel appropriate for the SDK: I would expect this to just return the source account on the transaction. Are these these methods Freighter-specific? I don't see the interface for what this.options.wallet is supposed to be.
There was a problem hiding this comment.
You can see the interface definition here https://github.com/stellar/js-stellar-sdk/pull/891/files#diff-d7db6307c66b5050519b2117d1ba8d0bbe3a743c92b463125f0315b713d4dccaR6-R24
Yes, it's Freighter-specific for now. Which is not a new problem. This code is mostly copied from the code that currently lives in the CLI repo, which is a bad place for it. Putting it here makes the problem better by exposing the problem and allowing us to fix it in a central place, within a JS repo instead of a Rust repo. We should be using whatever the Wallet standards team is coming up with.
The biggest change here from what's currently generated in the bindings is that you are now required to inject wallet, rather than defaulting to Freighter! A good improvement, I'm sure you'll agree! You can see that the current logic auto-imports Freighter if no wallet is injected.
| networkPassphrase: options.networkPassphrase, | ||
| }) | ||
| .addOperation(contract.call(options.method, ...(options.args ?? []))) | ||
| .setTimeout(TimeoutInfinite) |
There was a problem hiding this comment.
this was mentioned in the CLI meeting today - just calling out for posterity that we likely want to implement a bound here
There was a problem hiding this comment.
Yes, I was thinking this could be done in a follow-up PR, after the initial shape of the code lands here. This is not adding anything new to the logic used by bindings-ts, it's only exposing it more to people who know better.
There was a problem hiding this comment.
Sounds good to me 👍. We also need to land on the appropriate timebound defaults.
|
Coming back to this after the break, I see a little bit more about where this is coming from and where it's trying to go. I discussed this a bit with @mollykarcher and I reiterated that this feels like something that belongs at a layer above the SDK, maybe still under the Stellar GitHub umbrella but not even necessarily so? My rationale is that:
So I'd like to throw out the idea that this whole piece of functionality belongs in a separate repository, which can then be utilized by the ts-bindings in soroban-tools as needed. |
This type of functionality seems core to the js-stellar-sdk though. Building invocations, simulating them, signing them (and their soroban auths), these things are all core capabilities to interacting with Stellar now that it has support for contracts. Could you list out which functionality you'd like to see live elsewhere vs here?
Will there be a bidirectional relationship? I think the code that the CLI generates will be dependent on the js-stellar-sdk, but not vice versa. Is that case?
@Shaptic Is this ☝🏻 the main part you're concerned about for the bidirectional dependency? Any code we add here should be testable without the CLI, so we could focus on building out those tests without the CLI and any testing with the CLI could still be left over in the soroban-tools repo, until we figure out how best to handle testing these cross-repo things. |
Right, and these already exist.
I agree about grabbing on-chain contracts (this is already described in examples), I sort of agree about misc. XDR/WASM parsing (I don't like the idea of introducing dependencies to read WASM, though I don't know that that's required), but I don't agree about generating "contract clients".
I'm less concerned with the CLI stuff (I understand/hope that it's just more convenient than building the test infra from scratch using JS) and more with the reliance on a wallet interface. The SDK has always drawn a very clear line about not needing any relationship with a wallet. It relies exclusively on either (a) raw
Sure, good idea! I do think some of these make sense here, while others don't. I should have been more nuanced in my original critique, but a +2k LoC delta is a hard to digest quickly heh. I like a lot of the convenience that
|
|
Reading over this it seems that the biggest issue is with the wallet integration. I suggest that the ContractClient created from the sdk is just for encoding and decoding function arguments and return values. Then we can provide a higher level one that adds the wallet/transaction. Though I do think that an AssembledTransaction is a fundamental one if we strip away the wallet stuff. Furthermore some of the types defined in this PR should be moved to their own dependency. This way the generated TS can depend on that. This will allow multiple contract instances to agree on the types. |
|
@willemneal I think that's a great compromise! 👍 A tangential concern I realized is in response to @chadoh, above:
While I love the idea of putting it in a central place to iterate (and JS over Rust for sure lol), it also means we have way less flexibility in terms of introducing breaking changes. Since we probably still consider these as "unstable," if we want to iterate quickly on them, then this isn't a great place for that. Unless we explicitly want an But either way, I'm a big fan of keeping and refining the lower-level abstraction stuff here and moving the wallet/contract-specific type stuff out. |
Shaptic
changed the title
AssembledTransaction abstraction
chadoh
force-pushed
the
feat/contract-client-from-spec
branch
5 times, most recently
from
40c567f to
22c403b
Compare
- new e2e tests copied from cli `ts-tests` for the generated bindings, but with TypeScript removed because the ContractClient is generated here dynamically at run time, so we cannot know the types at compile time in the tests. - generate JSON specs from local .wasm files during initiaze.sh instead of generating TS bindings. As explained in the new wasms/specs/README, this is a bummer, but is temporary
ContractClient is now fully tested in `tests/e2e` rather than in this file Keeping this in a separate commit in case we decide it's better to go back and do things "The tests/unit way" instead of the new tests/e2e way. It feels maybe silly to have both.
chadoh
force-pushed
the
feat/contract-client-from-spec
branch
from
22c403b to
af361a5
Compare
|
Looks like the failing tests are also failing on |
|
Oh, I can't merge. Can you, @Shaptic? |
* Add generation of contract clients and an `AssembledTransaction` abstraction (#891) - new e2e tests copied from cli `ts-tests` for the generated bindings, but with TypeScript removed because the ContractClient is generated here dynamically at run time, so we cannot know the types at compile time in the tests. - generate JSON specs from local .wasm files during initiaze.sh instead of generating TS bindings. As explained in the new wasms/specs/README, this is a bummer, but is temporary * Update soroban-cli and sync with upstream `master` (#911) --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: George <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Make simulation optional, simplify wallet/signer interface (#921) * Update examples to use new module and package structure. (#900) * Fixup deprecation to specify exact version * Upgrade references to use latest modules * Bump follow-redirects from 1.15.3 to 1.15.4 (#906) Bumps [follow-redirects](https://github.com/follow-redirects/follow-redirects) from 1.15.3 to 1.15.4. - [Release notes](https://github.com/follow-redirects/follow-redirects/releases) - [Commits](follow-redirects/follow-redirects@v1.15.3...v1.15.4) --- updated-dependencies: - dependency-name: follow-redirects dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Export the individual event response instance (#904) * Add support for new `sendTransaction` response field (#905) * Add checks to ensure incorrect fields don't sneak in * Update README to flow better (#907) * Prepare v11.2.0 for release (#908) * Upgrade all dependencies besides chai * Add changelog entries * Eliminating `utility-types` dependency entirely (#912) Eliminated the 'utility-types' package since its functionalities are likely replaced by native TypeScript features. This change includes cleaning up imports and references in the codebase and updating the package.json and yarn.lock accordingly, resulting in a leaner dependency graph and potentially reducing installation times and package size. Co-authored-by: Sérgio Luis <[email protected]> * Release v11.2.1 (#913) * Upgrade dependencies and stellar-base * fix: stop using TimeoutInfinite * optional simulate & wallet, editable TransactionBuilder - Can now pass an `account` OR `wallet` when constructing the ContractClient, or none! If you pass none, you can still make view calls, since they don't need a signer. You will need to pass a `wallet` when calling things that need it, like `signAndSend`. - You can now pass `simulate: false` when first creating your transaction to skip simulation. You can then modify the transaction using the TransactionBuilder at `tx.raw` before manually calling `simulate`. Example: const tx = await myContract.myMethod( { args: 'for', my: 'method', ... }, { simulate: false } ); tx.raw.addMemo(Memo.text('Nice memo, friend!')) await tx.simulate(); - Error types are now collected under `AssembledTransaction.Errors` and `SentTransaction.Errors`. * Ensure that event streaming tests write a valid stream (#917) * Release v11.2.2 (#918) * export ExampleNodeWallet from lib Tyler van der Hoeven thought this would be useful. The current place it's exported from is surpassingly silly. But it functions properly and the tests fail the same way they failed before. * Drop all usage of array-based passing (#924) * feat(e2e-tests): new account & contract per test - New `clientFor` that instantiates a ContractClient for given contract, as well as initializes a new account, funding it with friendbot - Can also use `generateFundedKeypair` directly, as with test-swap - Stop generating anything in initialize.sh. Just check that the network is running and the pinned binary is installed, and fund the `$SOROBAN_ACCOUNT`. Ideally we wouldn't use the binary at all, but for now the tests are still shelling out to the CLI, so it's worth keeping the pinning around * wallet/signer only needs three methods * feat: no more `Wallet` interface Instead, just accept the things that Wallet contained. This avoids the conundrum of what to call the thing. - `Wallet` seems too high-level. Too high-level to be the concern of stellar-sdk, and too high-level for the thing being described here. It's really just two functions: `signTransaction` and `signAuthEntry`. - No need for this thing to defined `getPublicKey`, let alone any of the more complicated wrappers around it that it used to. Just have people pass in a `publicKey`. For convenience' sake, I also allowed this to be a Promise of a string, so that you don't need to instantiate ContractClient asynchronously, instead doing something like: new ContractClient({ publicKey: asyncPublicKeyLookupFromWallet(), ... }) This helps when getting public keys in a browser environment, where public key lookup is async, and adds little complexity to the logic here. * rm getAccount from exposed interface * make simulation public; wrap comments * explicit allowHttp * test(ava): set timeout to 2m * build: move ExampleNodeWallet to own entrypoint No need to pollute the global API or bundle size with this. * build: move ContractClient & AssembledTransaction These are a bit higher-level and experimental, at this point, so let's not clutter the global API or the bundle size unless people really want it. * fix: allow overriding 'publicKey' for 'signAuthEntries' * feat(contract-client): require publicKey * fix: use Networks from stellar-base * doc: explain 'errorTypes' param * build: ContractClient-related things in one dir * typo * move primitive type defs to contractclient * rm ContractClient.generate; do it in constructor * feat: separate rust_types to own import path * feat: don't make people import and use Networks enum I personally find TS enums a little surprising to work with, and my own codebases already have network passphrases littered throughout. I think we can upgrade to use the enum later, after more discussion about the exact interface. Let's not tangle that up in this change. * doc: include rust_types readme info in build the README.md file is not included in the `lib/rust_types` built version, so it's better to include it in a file that people can find by using the go-to-definition function in their editor, such as a `rust_types.ts` file directly, which gets built as `lib/rust_types.d.ts`. * build: make it easier to import rust_types * feat: basicNodeSigner as a plain-object factory Our suggested approach of spreading `signer` into `ContractClient` constructors causes typing issues, since `networkPassphrase` is a private field inside BasicNodeSigner. This means the `signer` needs to be spread in before the inclusion of `networkPassphrase`, otherwise it gets overwritten with `undefined` (or maybe TypeScript just thinks it will get overwritten). --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: George <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Sérgio Luis <[email protected]> Co-authored-by: Sérgio Luis <[email protected]> * fix(contract-client): stop jsifying method names This implementation needs to match what is done in the TS Bindings in Rust. Keeping "JSification" logic consistent in both is not worth the slight nicety of allowing people to type camelCaseMethodNames in JS. Additionally, having camelCaseMethodNames in one context when the real method name is probably_snake_case could lead to confusion. If someone types a camelCaseName in their CLI, the CLI will complain, and they might not know what's going on. * docs(contract-client): clean api, write a book Yes, a whole book about AssembledTransaction. It needed documentation; why not make it useful. This also removes an obsolute method, marks a couple as private, adds detail to other comments, fixes the `fee` type, updates SentTransaction docs, and organizes the code a bit. --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: George <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Sérgio Luis <[email protected]> Co-authored-by: Sérgio Luis <[email protected]>
Port ContractClient and AssembledTransaction from bindings-ts logic in CLI
Creates a new
tests/e2edirectory using the same AVA setup & most of the same tests as the ts-tests in the CLI. It feels a little suboptimal to have two separate testing configurations in the same project, but the new AVA tests are very fast compared to the others and run in their own CI workflow. I'm happy to discuss how to clean this up more, but wanted to start getting other eyes on this change sooner rather than later.The idea here is that most of the logic that's currently sequestered over in the
soroban contract bindings typescriptcan now be used more generally, and live closer to the otherstellar-sdklogic it interacts with. You can generate a contract dynamically, at run time. In follow-up PRs, I'll make it easier to grab on-chain contracts, extract the XDR from the custom section, and generate these contract clients.This will allow us to simplify the generated bindings to only include types, which cannot be generated at runtime.