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(bindings-ts)!: require configuring network settings #852

Merged
merged 18 commits into from
Aug 18, 2023

Conversation

chadoh
Copy link
Contributor

@chadoh chadoh commented Aug 9, 2023

What

Generated libraries now export a Contract class that needs to be instantiated by the user with RPC URL, Network Passphrase, and contract ID.

They also export a networks object to help initialize networkPassphrase and contractId with known-valid settings.

Surprise bonus: adds a ts-tests directory with a test for all test_custom_types.wasm exported methods! This exposed a couple easy-to-fix problems that were addressed, while marking other tests as known failures, which we can circle back to later. While adding these tests I also discovered some weird behavior. This is documented in the test cases; we can discuss how to best fix this behavior later.

Why

Keeps libraries focused on behavior, not specific endpoints, which will make them more flexible. Still gives contract authors a way to tell users about known supported networks.

Known limitations

To do:

Right now the new tests are not incorporated into CI at all. Do we want to incorporate them into CI? How? It might require running a localnet node within CI and deploying the contract to it. This seems like a lot of work! Is there a project with a similar setup we can copy?

@chadoh chadoh marked this pull request as ready for review August 11, 2023 17:52
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.

quick pass, not a formal review as I'm less familiar with this side

@kalepail
Copy link
Contributor

What would a cli command now look like? I read the README but it's still not clear how to set the networks object with custom networks

@kalepail kalepail removed their request for review August 14, 2023 13:37
@chadoh
Copy link
Contributor Author

chadoh commented Aug 14, 2023

@tyvdh for now, there is no way to automatically generate a library with multiple configured networks. Library authors will need to manually update the networks constant in index.ts. Do you think that's acceptable, as a starting point?

Expanding the CLI command to accept multiple --network and --contract-id arguments would require significant work, and might not be useful enough to warrant that work.

@chadoh chadoh force-pushed the feat/configurable-libs branch 2 times, most recently from 5d47a6e to b6eefd1 Compare August 15, 2023 19:24
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.

LGTM from my perspective—yeet it

@chadoh chadoh enabled auto-merge August 17, 2023 17:59
@chadoh chadoh disabled auto-merge August 17, 2023 17:59
chadoh and others added 11 commits August 17, 2023 14:00
Generated libraries now export a `Contract` class that needs to be
instantiated by the user with RPC URL, Network Passphrase, and contract
ID.
this logic was totally broken! thanks for showing us, tests.
you can see here that

- the input needs the `as const`
- but to make that work for complex types, a `readonly` is needed in the
  typing

additionally, the input and output are typed differently:

- the input needs to be wrapped in an array
- the output does not

also, for enums that do not have `values`, you still need to specify
`values: undefined`
When we exported functions for each method, we needed to make sure we
only used safe JS words. This no longer matters, since they're exposed
as methods on the `Contract` class.

Since this change required rebuilding the snapshot, I also made sure the
`readonly` settings were added to the source, rather than manually to
the snapshot.
@paulbellamy paulbellamy enabled auto-merge (squash) August 18, 2023 12:45
@paulbellamy paulbellamy merged commit 8d3f813 into stellar:main Aug 18, 2023
19 of 20 checks passed
willemneal added a commit to AhaLabs/stellar-cli that referenced this pull request Aug 21, 2023
* feat(bindings-ts)!: require configuring network settings

Generated libraries now export a `Contract` class that needs to be
instantiated by the user with RPC URL, Network Passphrase, and contract
ID.

* feat(bindings-ts): add test config for generated fixtures

* feat: test drive error-parsing in client libs

this logic was totally broken! thanks for showing us, tests.

* chore: add failing custom type test

* chore: demonstrate complex enum shortcomings

you can see here that

- the input needs the `as const`
- but to make that work for complex types, a `readonly` is needed in the
  typing

additionally, the input and output are typed differently:

- the input needs to be wrapped in an array
- the output does not

also, for enums that do not have `values`, you still need to specify
`values: undefined`

* chore: demonstrate many struct encoding problems

* chore: demonstrate many struct encoding problems

* feat: no more need for `Method` suffix

When we exported functions for each method, we needed to make sure we
only used safe JS words. This no longer matters, since they're exposed
as methods on the `Contract` class.

Since this change required rebuilding the snapshot, I also made sure the
`readonly` settings were added to the source, rather than manually to
the snapshot.

* chore: finish testing all custom_type methods

* chore: remove placeholder signing machinery

* feat: add `networks` export to generated library

* chore: update generated README

* chore: fix formatting of generated file

* build: appease clippy

* docs: remove outdated '--name' from example

* docs: fix example in doc comment

Re: https://typedoc.org/tags/example/, it is best to use both `@example`
and the triple-backtick code block

* build: update snapshot build command

* docs: improve method-options TSDoc

---------

Co-authored-by: Willem Wyndham <[email protected]>
@willemneal willemneal deleted the feat/configurable-libs branch October 20, 2023 21:00
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.

5 participants