Skip to content
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

feat: add ContractClient for easy contract interaction #929

Merged
merged 6 commits into from
Mar 20, 2024

Conversation

chadoh
Copy link
Contributor

@chadoh chadoh commented Mar 14, 2024

This is a port of the bulk of the TS Bindings logic to stellar-sdk. It is used by stellar/stellar-cli#1258, which you can see removes about the same number of lines from soroban-cli that this adds to stellar-sdk.

This allows similar argument- and result-parsing logic to dynamic frontend contexts that TS Bindings have enjoyed in TS contexts, while also delivering significant cleanup to the interfaces. It helps to put TS code in a TS codebase!

There are many things we would still like to do here:

  • Stop requiring a spec to be passed to the ContractClient, which still more-or-less forces a dependency on the CLI to generate this spec. Instead, we would like to provide the option to initialize a ContractClient from a contract ID. It can then fetch the on-chain contract, extract the XDR from the wasm's custom section, and parse the spec on the fly.
  • Make it easier to understand expired state errors and to extend the lifetime of relevant data.
  • Remove the JSON serialization/deserialization. This was a first-pass at supporting multi-auth workflows, and is not needed by most use-cases. For multi-auth workflows, we want to support a full-XDR serialization approach, rather than unexpectedly using JSON in this one Soroban use-case.
  • Continue shipping important improvements to the developer experience of the ContractClient. It's a solid start right now, but there are many ways to help developers build solid mental models about how contract development works on Stellar, and to help them troubleshoot issues as they build their contracts.
  • Consolidate tests. Right now, I dropped tests that had been in the CLI repo into this one. These new ones live in tests/e2e and use AVA, whereas the old tests use Mocha. It's kind of silly to have both. AVA and the dotenv setup it uses is also the main cause of the Socket Security reports below. I believe that this causes no real security issue for stellar-sdk users, since these are devDependencies.

However, none of these are blockers. Right now, we have an out-of-date developer experience in the TS Bindings, as well as largely-duplicate codebases between the CLI and this PR. This change avoids bloating the bundle size or exposed API of stellar-sdk by putting ContractClient in its own entrypoint, which makes it a safe starting point to merge. We can ship further improvements from here.

chadoh and others added 5 commits January 5, 2024 13:37
…raction (stellar#891)

* feat: spec.generateContractClient; AssembledTx

- 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

* chore: rollback test/contract_spec changes

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.
* Update examples to use new module and package structure. (stellar#900)
* Bump follow-redirects from 1.15.3 to 1.15.4 (stellar#906)
* Export the individual event response instance (stellar#904)
* Add support for new `sendTransaction` response field (stellar#905)
* Update README to flow better (stellar#907)
* Prepare v11.2.0 for release (stellar#908)

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: George <Shaptic@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* Update examples to use new module and package structure. (stellar#900)

* Fixup deprecation to specify exact version
* Upgrade references to use latest modules

* Bump follow-redirects from 1.15.3 to 1.15.4 (stellar#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] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Export the individual event response instance (stellar#904)

* Add support for new `sendTransaction` response field (stellar#905)

* Add checks to ensure incorrect fields don't sneak in

* Update README to flow better (stellar#907)

* Prepare v11.2.0 for release (stellar#908)

* Upgrade all dependencies besides chai
* Add changelog entries

* Eliminating `utility-types` dependency entirely (stellar#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 <sergiocl@airtm.io>

* Release v11.2.1 (stellar#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 (stellar#917)
* Release v11.2.2 (stellar#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 (stellar#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] <support@github.com>
Co-authored-by: George <Shaptic@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Sérgio Luis <sergiolclem@gmail.com>
Co-authored-by: Sérgio Luis <sergiocl@airtm.io>
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.
* master:
  Move TypeScript to devDependencies to reduce bundle size (stellar#926)
  Drop all usage of array-based passing (stellar#924)
  Release v11.2.2 (stellar#918)
  Ensure that event streaming tests write a valid stream (stellar#917)
  Release v11.2.1 (stellar#913)
  Eliminating `utility-types` dependency entirely (stellar#912)
  Prepare v11.2.0 for release (stellar#908)
  Update README to flow better (stellar#907)
  Add support for new `sendTransaction` response field (stellar#905)
  Export the individual event response instance (stellar#904)
  Bump follow-redirects from 1.15.3 to 1.15.4 (stellar#906)
  Update examples to use new module and package structure. (stellar#900)
@chadoh chadoh marked this pull request as ready for review March 14, 2024 14:46
Copy link

socket-security bot commented Mar 14, 2024

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/acorn-walk@8.3.1 None 0 52.3 kB marijn
npm/ava@5.3.1 Transitive: environment, eval, filesystem, unsafe +121 5.95 MB novemberborn
npm/dotenv@16.3.1 environment, filesystem 0 71.6 kB motdotla

View full report↗︎

Copy link

socket-security bot commented Mar 14, 2024

🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎

To accept the risk, merge this PR and you will not be notified again.

Alert Package NoteSource
Debug access npm/stack-utils@2.0.6
Filesystem access npm/dotenv@16.3.1

View full report↗︎

Next steps

What is debug access?

Uses debug, reflection and dynamic code execution features.

Removing the use of debug will reduce the risk of any reflection and dynamic code execution.

What is filesystem access?

Accesses the file system, and could potentially read sensitive data.

If a package must read the file system, clarify what it will read and ensure it reads only what it claims to. If appropriate, packages can leave file system access to consumers and operate on data passed to it instead.

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of ecosystem/package-name@version specifiers. e.g. @SocketSecurity ignore npm/foo@1.0.0 or ignore all packages with @SocketSecurity ignore-all

  • @SocketSecurity ignore npm/stack-utils@2.0.6
  • @SocketSecurity ignore npm/dotenv@16.3.1

@@ -28,12 +28,13 @@
"build:prod": "cross-env NODE_ENV=production yarn _build",
"build:node": "yarn _babel && yarn build:ts",
"build:ts": "tsc -p ./config/tsconfig.json",
"build:test": "tsc -p ./test/tsconfig.json",
"build:test": "tsc -p ./test/unit/tsconfig.json",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was added as part of an early-on change when the new AVA tests compiled with a separate tsconfig. They no longer use TS at all, for now. So I could put this back. Is it worth it?

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.
Copy link
Contributor

@Shaptic Shaptic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this LGTM! Especially since most of it was reviewed already in previous PRs. But shouldn't we be merging the stellar:tsbindings branch into stellar:master?

@Shaptic Shaptic merged commit e66112d into stellar:master Mar 20, 2024
9 of 10 checks passed
@chadoh chadoh deleted the feat/contract-client branch May 3, 2024 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants