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!: export ContractClient et al. in contract; rename SorobanRpc to rpc #962

Merged
merged 2 commits into from
May 15, 2024

Conversation

chadoh
Copy link
Contributor

@chadoh chadoh commented May 9, 2024

Breaking Changes

  • ContractClient functionality previously added in v11.3.0 was exported in a non-standard way. You can now import it as any other stellar-sdk module.

    - import { ContractClient } from '@stellar/stellar-sdk/lib/contract_client'
    + import { contract } from '@stellar/stellar-sdk'
    + const { Client } = contract

    Note that this top-level contract export is a container for ContractClient and related functionality. The ContractClient class is now available at contract.Client, as shown. Further note that there is a capitalized Contract export as well, which comes from stellar-base. You can remember which is which because capital-C Contract is a class, whereas lowercase-c contract is a container/module with a bunch of classes, functions, and types.

    Additionally, this is available from the /contract entrypoint, if your version of Node and TypeScript support the exports declaration. Finally, some of its exports have been renamed.

     import {
    -  ContractClient,
    +  Client,
       AssembledTransaction,
    -  ContractClientOptions,
    +  ClientOptions,
       SentTransaction,
    -} from '@stellar/stellar-sdk/lib/contract_client'
    +} from '@stellar/stellar-sdk/contract'
  • The ContractSpec class is now nested under the contract module, and has been renamed to Spec.

    -import { ContractSpec } from '@stellar/stellar-sdk'
    +import { contract } from '@stellar/stellar-sdk'
    +const { Spec } = contract

    Alternatively, you can import this from the contract entrypoint, if your version of Node and TypeScript support the exports declaration.

    -import { ContractSpec } from '@stellar/stellar-sdk'
    +import { Spec } from '@stellar/stellar-sdk/contract'
  • Previously, AssembledTransaction.signAndSend() would return a SentTransaction even if the transaction never finalized. That is, if it successfully sent the transaction to the network, but the transaction was still status: 'PENDING', then it would console.error an error message, but return the indeterminate transaction anyhow.

    It now throws a SentTransaction.Errors.TransactionStillPending error with that error message instead.

Deprecated

  • SorobanRpc module is now also exported as rpc. You can import it with either name for now, but SorobanRpc will be removed in a future release.

     import { SorobanRpc } from '@stellar/stellar-sdk'
    +// OR
    +import { rpc } from '@stellar/stellar-sdk'

    You can also now import it at the /rpc entrypoint, if your version of Node and TypeScript support the exports declaration.

    -import { SorobanRpc } from '@stellar/stellar-sdk'
    -const { Api } = SorobanRpc
    +import { Api } from '@stellar/stellar-sdk/rpc'

Other

  • This PR started as an attempt to appease eslint for the contract directory (previously the contract_client directory). While this turned out to be a bit premature (given that much other code in this repo doesn't pass our eslint rules), it's still nice cleanup. That's why you'll see .eslintrc.js changes and related code updates. Some notes on all this:
    • We had .eslintrc.js, src/.eslintrc.js, and src/soroban/.eslintrc.js. The second two were very similar. I made them identical and removed the more deeply nested one.
    • We have all TypeScript in src, but were only using Airbnb's non-TypeScript recommended settings. They recommend using both, for TypeScript projects. That's what I've done.
    • The valid-jsdoc rule was deprecated. They now recommend using the eslint-plugin-jsdoc. I've done that, and extended their recommended rules. I've overridden some rules. For example, I researched and found that documenting function params with @param doesn't actually show up in your in-editor typeahead/mouseover. So I turned off the rule that requires these. We should prefer TSDoc for params instead. I've added these throughout.
  • I attempted to add some JSDoc comments to the export * as blah from 'whatever' lines in src/index.ts. These don't actually show up when you import these objects. We will need to research this more when we do a larger module reorganization later. For now, they're useful notes.

src/.eslintrc.js Show resolved Hide resolved
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.

absolutely Herculean 👏 coupla requests ⬇️

src/contract_client/basic_node_signer.ts Outdated Show resolved Hide resolved
src/contract_client/basic_node_signer.ts Outdated Show resolved Hide resolved
src/contract_client/types.ts Show resolved Hide resolved
src/contract_client/utils.ts Outdated Show resolved Hide resolved
.eslintrc.js Outdated Show resolved Hide resolved
Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

I defer to @Shaptic, just one question from me.

src/contract_client/sent_transaction.ts Show resolved Hide resolved
@chadoh chadoh force-pushed the chore/lint-contract-client branch from 9b55dce to 0832590 Compare May 10, 2024 17:02
Copy link

socket-security bot commented May 10, 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/builtin-modules@3.3.0
Filesystem access npm/eslint-plugin-jsdoc@48.2.4

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/builtin-modules@3.3.0
  • @SocketSecurity ignore npm/eslint-plugin-jsdoc@48.2.4

chadoh added a commit to AhaLabs/js-stellar-sdk that referenced this pull request May 10, 2024
This includes some fixes and updates to eslint configuration to make it
work as expected

- Extended `airbnb-typescript/base` to get it to stop yelling at me
  about importing files without file extension. Saw this recommended as
  the fix on StackOverflow [[1]]. And it makes sense to me that if we are
  extending Airbnb's lint rules and using TypeScript, we probably want
  their TypeScript-specific lint rules, too.
- Added the `eslint-plugin-jsdoc` plugin because the old `valid-jsdoc`
  rule we were using has been deprecated [[2]], and this plugin is the new
  way. Previously we had `valid-jsdoc: 1` (with some extra customization),
  and my guess is that extending `plugin:jsdoc/recommended` (plus some
  customization) is roughly equivalent.
- Researched [[3]] whether JSDoc `@param`-style docs or TSDoc-style
  `/** inline param docs */` work better. TSDoc work better. So disabled
  `jsdoc/require-param`.

  [1]: https://stackoverflow.com/a/67610259/249801
  [2]: https://eslint.org/docs/latest/rules/valid-jsdoc
  [3]: stellar#962 (comment)
@chadoh chadoh force-pushed the chore/lint-contract-client branch from 0832590 to 31e6123 Compare May 10, 2024 17:08
chadoh added a commit to AhaLabs/js-stellar-sdk that referenced this pull request May 10, 2024
This includes some fixes and updates to eslint configuration to make it
work as expected

- Extended `airbnb-typescript/base` to get it to stop yelling at me
  about importing files without file extension. Saw this recommended as
  the fix on StackOverflow [[1]]. And it makes sense to me that if we are
  extending Airbnb's lint rules and using TypeScript, we probably want
  their TypeScript-specific lint rules, too.
- Added the `eslint-plugin-jsdoc` plugin because the old `valid-jsdoc`
  rule we were using has been deprecated [[2]], and this plugin is the new
  way. Previously we had `valid-jsdoc: 1` (with some extra customization),
  and my guess is that extending `plugin:jsdoc/recommended` (plus some
  customization) is roughly equivalent.
- Researched [[3]] whether JSDoc `@param`-style docs or TSDoc-style
  `/** inline param docs */` work better. TSDoc work better. So disabled
  `jsdoc/require-param`.

  [1]: https://stackoverflow.com/a/67610259/249801
  [2]: https://eslint.org/docs/latest/rules/valid-jsdoc
  [3]: stellar#962 (comment)
@chadoh chadoh force-pushed the chore/lint-contract-client branch from 31e6123 to c01532d Compare May 10, 2024 17:20
chadoh added a commit to AhaLabs/js-stellar-sdk that referenced this pull request May 10, 2024
This includes some fixes and updates to eslint configuration to make it
work as expected

- Extended `airbnb-typescript/base` to get it to stop yelling at me
  about importing files without file extension. Saw this recommended as
  the fix on StackOverflow [[1]]. And it makes sense to me that if we are
  extending Airbnb's lint rules and using TypeScript, we probably want
  their TypeScript-specific lint rules, too.
- Added the `eslint-plugin-jsdoc` plugin because the old `valid-jsdoc`
  rule we were using has been deprecated [[2]], and this plugin is the new
  way. Previously we had `valid-jsdoc: 1` (with some extra customization),
  and my guess is that extending `plugin:jsdoc/recommended` (plus some
  customization) is roughly equivalent.
- Researched [[3]] whether JSDoc `@param`-style docs or TSDoc-style
  `/** inline param docs */` work better. TSDoc work better. So disabled
  `jsdoc/require-param`.

  [1]: https://stackoverflow.com/a/67610259/249801
  [2]: https://eslint.org/docs/latest/rules/valid-jsdoc
  [3]: stellar#962 (comment)
@chadoh chadoh force-pushed the chore/lint-contract-client branch from c01532d to 3e90f84 Compare May 10, 2024 17:21
@chadoh
Copy link
Contributor Author

chadoh commented May 10, 2024

Didn't realize just how unhappy ESLint is with basically all the code in this repo 🤔

I saw that src/.eslintrc.js and src/soroban/.eslintrc.js were basically duplicates of each other. I tweaked them to match (removed the no-unused-vars setting, to make that an error case, which seems like what we actually want? One had been set to warn and the other had disabled it.), then removed src/soroban/.eslintrc.js.

I also fixed some config issues that I introduced earlier, so you can lint the whole repo again. And find that we have:

2081 problems (1063 errors, 1018 warnings)

¯\_(ツ)_/¯

Good enough for now, I guess??

chadoh added a commit to AhaLabs/js-stellar-sdk that referenced this pull request May 10, 2024
This includes some fixes and updates to eslint configuration to make it
work as expected

- Extended `airbnb-typescript/base` to get it to stop yelling at me
  about importing files without file extension. Saw this recommended as
  the fix on StackOverflow [[1]]. And it makes sense to me that if we are
  extending Airbnb's lint rules and using TypeScript, we probably want
  their TypeScript-specific lint rules, too.
- Added the `eslint-plugin-jsdoc` plugin because the old `valid-jsdoc`
  rule we were using has been deprecated [[2]], and this plugin is the new
  way. Previously we had `valid-jsdoc: 1` (with some extra customization),
  and my guess is that extending `plugin:jsdoc/recommended` (plus some
  customization) is roughly equivalent.
- Researched [[3]] whether JSDoc `@param`-style docs or TSDoc-style
  `/** inline param docs */` work better. TSDoc work better. So disabled
  `jsdoc/require-param`.
- Remove mostly-duplicate `src/soroban/.eslintrc.js`, which had only one
  setting different from `src/.eslintrc.js`

Note that `src/contract_client` is now perhaps the only directory of
code to pass our ESLint settings 🤔

  [1]: https://stackoverflow.com/a/67610259/249801
  [2]: https://eslint.org/docs/latest/rules/valid-jsdoc
  [3]: stellar#962 (comment)
@chadoh chadoh force-pushed the chore/lint-contract-client branch from 3e90f84 to 2ee6126 Compare May 10, 2024 19:28
@chadoh chadoh changed the title chore(contract-client): appease eslint feat: export SorobanRpc.ContractClient May 10, 2024
src/contract_spec.ts Show resolved Hide resolved
src/soroban/contract_client/index.ts Outdated Show resolved Hide resolved
src/soroban/index.ts Outdated Show resolved Hide resolved
@chadoh chadoh force-pushed the chore/lint-contract-client branch 6 times, most recently from 654af31 to bc8df30 Compare May 14, 2024 15:45
@chadoh chadoh changed the title feat: export SorobanRpc.ContractClient feat!: export ContractClient; rename SorobanRpc to Rpc May 14, 2024
@chadoh chadoh force-pushed the chore/lint-contract-client branch from bc8df30 to 798b4b6 Compare May 14, 2024 19:03
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.

A few small notes but otherwise this is 🔥 🔥 🔥 great work! 👏

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
src/contract_client/sent_transaction.ts Show resolved Hide resolved
chadoh added a commit to AhaLabs/soroban-cli that referenced this pull request May 14, 2024
Uses the in-progress branch at
stellar/js-stellar-sdk#962, which will be merged
and released as 12rc3 soon. This PR can be updated to use the release at
that point.

Note that using the new `import { Client } from
'@stellar/stellar-sdk/ContractClient` entrypoint requires getting rid
of the "build the TS Bindings for both Common JS and ES Modules"
behavior we had before. I think this is fine. From [TypeScript's
`moduleResolution`
documentation](https://www.typescriptlang.org/docs/handbook/modules/theory.html#module-resolution-for-libraries):

> When compiling a library, you don’t know where the output code will
> run, but you’d like it to run in as many places as possible. Using
> `"module": "nodenext"` (along with the implied `"moduleResolution":
> "nodenext"`) is the best bet for maximizing the compatibility of the
> output JavaScript’s module specifiers, since it will force you to comply
> with Node.js’s stricter rules for import module resolution.

We also don't really need these generated NPM modules to be more
flexible than stellar-sdk itself, and it doesn't do the
build-to-cjs-and-esm thing.
Copy link

socket-security bot commented May 14, 2024

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

Package New capabilities Transitives Size Publisher
npm/eslint-config-airbnb-typescript@18.0.0 Transitive: filesystem +14 5.38 MB iamturns
npm/eslint-plugin-jsdoc@48.2.4 filesystem Transitive: unsafe +11 2.77 MB gajus

View full report↗︎

@Shaptic Shaptic changed the title feat!: export ContractClient; rename SorobanRpc to Rpc feat!: export ContractClient et al. in contracts; rename SorobanRpc to rpc May 14, 2024
@Shaptic Shaptic mentioned this pull request May 14, 2024
4 tasks
chadoh added a commit to AhaLabs/js-stellar-sdk that referenced this pull request May 14, 2024
This includes some fixes and updates to eslint configuration to make it
work as expected

- Extended `airbnb-typescript/base` to get it to stop yelling at me
  about importing files without file extension. Saw this recommended as
  the fix on StackOverflow [[1]]. And it makes sense to me that if we are
  extending Airbnb's lint rules and using TypeScript, we probably want
  their TypeScript-specific lint rules, too.
- Added the `eslint-plugin-jsdoc` plugin because the old `valid-jsdoc`
  rule we were using has been deprecated [[2]], and this plugin is the new
  way. Previously we had `valid-jsdoc: 1` (with some extra customization),
  and my guess is that extending `plugin:jsdoc/recommended` (plus some
  customization) is roughly equivalent.
- Researched [[3]] whether JSDoc `@param`-style docs or TSDoc-style
  `/** inline param docs */` work better. TSDoc work better. So disabled
  `jsdoc/require-param`.
- Remove mostly-duplicate `src/soroban/.eslintrc.js`, which had only one
  setting different from `src/.eslintrc.js`

Note that `src/contract_client` is now perhaps the only directory of
code to pass our ESLint settings 🤔

  [1]: https://stackoverflow.com/a/67610259/249801
  [2]: https://eslint.org/docs/latest/rules/valid-jsdoc
  [3]: stellar#962 (comment)
@chadoh chadoh force-pushed the chore/lint-contract-client branch 3 times, most recently from 7862b46 to 1bcb161 Compare May 14, 2024 22:52
@chadoh chadoh force-pushed the chore/lint-contract-client branch 5 times, most recently from 2936139 to 49ae5bf Compare May 15, 2024 00:44
@chadoh chadoh changed the title feat!: export ContractClient et al. in contracts; rename SorobanRpc to rpc feat!: export ContractClient et al. in contract; rename SorobanRpc to rpc May 15, 2024
This includes some fixes and updates to eslint configuration to make it
work as expected

- Extended `airbnb-typescript/base` to get it to stop yelling at me
  about importing files without file extension. Saw this recommended as
  the fix on StackOverflow [[1]]. And it makes sense to me that if we are
  extending Airbnb's lint rules and using TypeScript, we probably want
  their TypeScript-specific lint rules, too.
- Added the `eslint-plugin-jsdoc` plugin because the old `valid-jsdoc`
  rule we were using has been deprecated [[2]], and this plugin is the new
  way. Previously we had `valid-jsdoc: 1` (with some extra customization),
  and my guess is that extending `plugin:jsdoc/recommended` (plus some
  customization) is roughly equivalent.
- Researched [[3]] whether JSDoc `@param`-style docs or TSDoc-style
  `/** inline param docs */` work better. TSDoc work better. So disabled
  `jsdoc/require-param`.
- Remove mostly-duplicate `src/soroban/.eslintrc.js`, which had only one
  setting different from `src/.eslintrc.js`

Note that `src/contract_client` is now perhaps the only directory of
code to pass our ESLint settings 🤔

  [1]: https://stackoverflow.com/a/67610259/249801
  [2]: https://eslint.org/docs/latest/rules/valid-jsdoc
  [3]: stellar#962 (comment)
@chadoh chadoh force-pushed the chore/lint-contract-client branch from e9fb8dc to 8bf93ea Compare May 15, 2024 19:16
@chadoh chadoh force-pushed the chore/lint-contract-client branch from 8bf93ea to 5eafd56 Compare May 15, 2024 19:18
- `SorobanRpc` now also exported as `rpc`

- New main export `contract`

  This allows us to import it the usual way, instead of needing to do
  things like

      import { ContractClient } from "stellar-sdk/lib/contract_client"

  which doesn't work in the browser (because `lib`)

- `ContractSpec` now available at `contract.Spec`

- Also includes other supporting classes, functions, and types:
  - `contract.AssembledTransaction`
  - `contract.ClientOptions`
  - etc

- These are also available at matching entrypoints, if your
node and TypeScript configuration support the `exports` declaration:

    import {
      Client,
      ClientOptions,
      AssembledTransaction,
    } from 'stellar-sdk/contract'

This also attempts to document exported modules. The JSDoc-style
comments in `src/index.ts` don't actually show up when you import these
things. We can figure this out later. Docs here
https://jsdoc.app/howto-es2015-modules. In the meantime, these are still
useful notes that we can use later.

Co-authored-by: George <Shaptic@users.noreply.github.com>
@chadoh chadoh force-pushed the chore/lint-contract-client branch 2 times, most recently from 78e13e0 to 7883dc3 Compare May 15, 2024 20:36
@Shaptic Shaptic merged commit 4b7939b into stellar:master May 15, 2024
16 of 17 checks passed
chadoh added a commit to AhaLabs/soroban-cli that referenced this pull request May 15, 2024
Uses the in-progress branch at
stellar/js-stellar-sdk#962, which will be merged
and released as 12rc3 soon. This PR can be updated to use the release at
that point.

Note that using the new `import { Client } from
'@stellar/stellar-sdk/contract` entrypoint requires getting rid
of the "build the TS Bindings for both Common JS and ES Modules"
behavior we had before. I think this is fine. From [TypeScript's
`moduleResolution`
documentation](https://www.typescriptlang.org/docs/handbook/modules/theory.html#module-resolution-for-libraries):

> When compiling a library, you don’t know where the output code will
> run, but you’d like it to run in as many places as possible. Using
> `"module": "nodenext"` (along with the implied `"moduleResolution":
> "nodenext"`) is the best bet for maximizing the compatibility of the
> output JavaScript’s module specifiers, since it will force you to comply
> with Node.js’s stricter rules for import module resolution.

We also don't really need these generated NPM modules to be more
flexible than stellar-sdk itself, and it doesn't do the
build-to-cjs-and-esm thing.
chadoh added a commit to AhaLabs/soroban-cli that referenced this pull request May 15, 2024
Uses the in-progress branch at
stellar/js-stellar-sdk#962, which will be merged
and released as 12rc3 soon. This PR can be updated to use the release at
that point.

Note that using the new `import { Client } from
'@stellar/stellar-sdk/contract` entrypoint requires getting rid
of the "build the TS Bindings for both Common JS and ES Modules"
behavior we had before. I think this is fine. From [TypeScript's
`moduleResolution`
documentation](https://www.typescriptlang.org/docs/handbook/modules/theory.html#module-resolution-for-libraries):

> When compiling a library, you don’t know where the output code will
> run, but you’d like it to run in as many places as possible. Using
> `"module": "nodenext"` (along with the implied `"moduleResolution":
> "nodenext"`) is the best bet for maximizing the compatibility of the
> output JavaScript’s module specifiers, since it will force you to comply
> with Node.js’s stricter rules for import module resolution.

We also don't really need these generated NPM modules to be more
flexible than stellar-sdk itself, and it doesn't do the
build-to-cjs-and-esm thing.
@chadoh chadoh deleted the chore/lint-contract-client branch May 15, 2024 20:56
@chadoh chadoh restored the chore/lint-contract-client branch May 15, 2024 20:58
chadoh added a commit to AhaLabs/soroban-cli that referenced this pull request May 16, 2024
Uses the in-progress branch at
stellar/js-stellar-sdk#962, which will be merged
and released as 12rc3 soon. This PR can be updated to use the release at
that point.

Note that using the new `import { Client } from
'@stellar/stellar-sdk/contract` entrypoint requires getting rid
of the "build the TS Bindings for both Common JS and ES Modules"
behavior we had before. I think this is fine. From [TypeScript's
`moduleResolution`
documentation](https://www.typescriptlang.org/docs/handbook/modules/theory.html#module-resolution-for-libraries):

> When compiling a library, you don’t know where the output code will
> run, but you’d like it to run in as many places as possible. Using
> `"module": "nodenext"` (along with the implied `"moduleResolution":
> "nodenext"`) is the best bet for maximizing the compatibility of the
> output JavaScript’s module specifiers, since it will force you to comply
> with Node.js’s stricter rules for import module resolution.

We also don't really need these generated NPM modules to be more
flexible than stellar-sdk itself, and it doesn't do the
build-to-cjs-and-esm thing.
@chadoh chadoh deleted the chore/lint-contract-client branch May 16, 2024 01:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants