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(ts-bindings)!: now powered by ContractClient #1258

Merged
merged 7 commits into from
Mar 26, 2024

Conversation

chadoh
Copy link
Contributor

@chadoh chadoh commented Mar 13, 2024

What

Most logic in the TS Bindings has now been implemented in stellar-sdk as
a ContractClient that dynamically fetches the contract's XDR and
generates JS for each contract method at runtime. This means logic like
AssembledTransaction now lives entirely in stellar-sdk, and can be
relied on by each library generated by TS Bindings, rather than
duplicating this logic in each of these libraries. TS Bindings now has a
much smaller focus—basically, only to generate the types.

This shrinks the TS Bindings, importing and using this new
ContractClient logic.

Why

Simplicity, code cleanliness, keeping JS in a JS library, making
dependencies easior to troubleshoot and improve

Known limitations

[TODO or N/A]

Most logic in the TS Bindings has now been implemented in stellar-sdk as
a `ContractClient` that dynamically fetches the contract's XDR and
generates JS for each contract method at runtime. This means logic like
`AssembledTransaction` now lives entirely in stellar-sdk, and can be
relied on by each library generated by TS Bindings, rather than
duplicating this logic in each of these libraries. TS Bindings now has a
much smaller focus—basically, only to generate the types.

This shrinks the TS Bindings, importing and using this new
ContractClient logic.
All tests that are being removed are now duplicates of tests that are in
stellar-sdk. So are the test-custom-types tests, but those ones actually
caught some edge cases, so maybe they're worth keeping.
@chadoh chadoh force-pushed the feat/ts-bindings-minimization branch from 0688a7e to 5161053 Compare March 21, 2024 14:50
@chadoh chadoh marked this pull request as ready for review March 21, 2024 14:50
@willemneal
Copy link
Member

@leighmcculloch The current failing action here doesn't seem to provide any info on the error, just that there was one.

Copy link
Member

@willemneal willemneal left a comment

Choose a reason for hiding this comment

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

Don't want to slow down merging this, but I am now noticing a lot of unwraps which is a bad code smell. I pointed out two places to remove them, but change functions to return Results is needed for the rest.

…ate.rs

Co-authored-by: Willem Wyndham <willem@ahalabs.dev>
@chadoh
Copy link
Contributor Author

chadoh commented Mar 21, 2024

Don't want to slow down merging this, but I am now noticing a lot of unwraps which is a bad code smell. I pointed out two places to remove them, but change functions to return Results is needed for the rest.

Other than the two you commented on, which have now been removed, I don't see any other new unwraps added here.

@chadoh
Copy link
Contributor Author

chadoh commented Mar 21, 2024

@leighmcculloch when I run the failing build's command locally, I get this output. No idea what to do with this information.

rustup run nightly rust-analyzer analysis-stats .
Database loaded:     2.46s, 0b (metadata 664.92ms, 0b; build 326.30ms, 0b)
  item trees: 104
Item Tree Collection: 35.56ms, 0b
  crates: 15, mods: 165, decls: 2648, bodies: 2698, adts: 170, consts: 82
Item Collection:     20.59s, 0b
11/2698 0% processing: test_udt::__Contract__7e9e5ac30f2216fd0fd6f5faed316f2d5983361a4203c3330cfa46ef65bb4767_ctor___rust_ctor___2/2698 0% processing: test_udt::__Contract__7e9e5ac30f2216fd0fd6f5faed316f2d5983361a4203c3330cfa46ef65bb4767_ctor___rust_ctor___c96/2698 3% processing: test_token::contract::__Token__923fa460c9919e4391337c06e7b59eb9d5d349a330d4238eb8c6a675ce9c11a1_ctor___rus7/2698 3% processing: test_token::contract::__Token__923fa460c9919e4391337c06e7b59eb9d5d349a330d4238eb8c6a675ce9c11a1_ctor___rust8/2698 3% processing: test_token::contract::__Token__844715cd2ff15151dacce5e827f634658fb394cf95ad4149191d6a4ffc9f7763_ctor___rust9/2698 3% processing: test_token::contract::__Token__844715cd2ff15151dacce5e827f634658fb394cf95ad4149191d6a4ffc9f7763_ctor___rust255/2698 9% processing: test_swap::__AtomicSwapContract__da47c2f450a4f9d538d86d600d55149afd39d6672fdd1f30c68ad5be21cadad8_ctor___6/2698 9% processing: test_swap::__AtomicSwapContract__da47c2f450a4f9d538d86d600d55149afd39d6672fdd1f30c68ad5be21cadad8_ctor___ru77/2698 10% processing: test_hello_world::__Contract__33a4d07b69a91f4816f9a522947da670f02f13ac2e951ebf5b90ac4474ad96a3_ctor___rus8/2698 10% processing: test_hello_world::__Contract__33a4d07b69a91f4816f9a522947da670f02f13ac2e951ebf5b90ac4474ad96a3_ctor___rust377/2698 13% processing: test_custom_types::__Contract__c0c352fcd73be18fba179babc6e2582992e079e69e10db16a55665c0862ccdf4_ctor___r8/2698 14% processing: test_custom_types::__Contract__c0c352fcd73be18fba179babc6e2582992e079e69e10db16a55665c0862ccdf4_ctor___rus2423/2698 89% processing: soroban_cli::commands::contract::init::tests::test_init_does_not_duplicate_frontend_readme_contents_whe4/2698 89% processing: soroban_cli::commands::contract::init::tests::test_init_does_not_duplicate_frontend_readme_contents_when_rBody lowering:       1.75s, 0b
11/2698 0% processing: test_udt::__Contract__7e9e5ac30f2216fd0fd6f5faed316f2d5983361a4203c3330cfa46ef65bb4767_ctor___rust_ctor___2/2698 0% processing: test_udt::__Contract__7e9e5ac30f2216fd0fd6f5faed316f2d5983361a4203c3330cfa46ef65bb4767_ctor___rust_ctor___c96/2698 3% processing: test_token::contract::__Token__923fa460c9919e4391337c06e7b59eb9d5d349a330d4238eb8c6a675ce9c11a1_ctor___rus7/2698 3% processing: test_token::contract::__Token__923fa460c9919e4391337c06e7b59eb9d5d349a330d4238eb8c6a675ce9c11a1_ctor___rust8/2698 3% processing: test_token::contract::__Token__844715cd2ff15151dacce5e827f634658fb394cf95ad4149191d6a4ffc9f7763_ctor___rust9/2698 3% processing: test_token::contract::__Token__844715cd2ff15151dacce5e827f634658fb394cf95ad4149191d6a4ffc9f7763_ctor___rust255/2698 9% processing: test_swap::__AtomicSwapContract__da47c2f450a4f9d538d86d600d55149afd39d6672fdd1f30c68ad5be21cadad8_ctor___6/2698 9% processing: test_swap::__AtomicSwapContract__da47c2f450a4f9d538d86d600d55149afd39d6672fdd1f30c68ad5be21cadad8_ctor___ru77/2698 10% processing: test_hello_world::__Contract__33a4d07b69a91f4816f9a522947da670f02f13ac2e951ebf5b90ac4474ad96a3_ctor___rus8/2698 10% processing: test_hello_world::__Contract__33a4d07b69a91f4816f9a522947da670f02f13ac2e951ebf5b90ac4474ad96a3_ctor___rust377/2698 13% processing: test_custom_types::__Contract__c0c352fcd73be18fba179babc6e2582992e079e69e10db16a55665c0862ccdf4_ctor___r8/2698 14% processing: test_custom_types::__Contract__c0c352fcd73be18fba179babc6e2582992e079e69e10db16a55665c0862ccdf4_ctor___rus2423/2698 89% processing: soroban_cli::commands::contract::init::tests::test_init_does_not_duplicate_frontend_readme_contents_whe4/2698 89% processing: soroban_cli::commands::contract::init::tests::test_init_does_not_duplicate_frontend_readme_contents_when_r  exprs: 93379, ??ty: 33 (0%), ?ty: 11 (0%), !ty: 0
  pats: 20628, ??ty: 1 (0%), ?ty: 0 (0%), !ty: 3
Inference:           19.83s, 0b
MIR lowering:        425.38ms, 0b
Mir failed bodies: 66 (3%)
Data layouts:        139.27ms, 0b
Failed data layouts: 4 (2%)
Const evaluation:    129.71ms, 0b
Failed const evals: 0 (0%)
Total:               42.90s, 0b

@willemneal willemneal enabled auto-merge (squash) March 22, 2024 18:42
@chadoh chadoh force-pushed the feat/ts-bindings-minimization branch from 9a516c2 to 43e38e5 Compare March 22, 2024 19:13
The history of this build is we added it in some lower level repos that had complex workspace setups (e.g. env and sdk) and we kept breaking rust-analyzer which was destroying our IDE productivity. Things have changed, the sdk repo is not as complex as it was, the env still is. But in any case none of the problems we had would ever happen in this repo.
@willemneal willemneal merged commit 58e4189 into stellar:main Mar 26, 2024
11 of 12 checks passed
@chadoh chadoh deleted the feat/ts-bindings-minimization branch April 2, 2024 17:14
@leighmcculloch
Copy link
Member

leighmcculloch commented Apr 26, 2024

Most logic in the TS Bindings has now been implemented in stellar-sdk as
a ContractClient that dynamically fetches the contract's XDR and
generates JS for each contract method at runtime.

👋🏻 Couple questions about this change:

Does this mean that use of the bindings always downloads the contract spec in the user browser? Or is the contract spec embedded and the type generation is just dynamic?

Is this change a breaking change? Does the interface change that a developer interacts with? Saw one crew running into issues after updating to v20.3.4 saying that the interface was radically different.

@shan-57blocks
Copy link

shan-57blocks commented Apr 26, 2024

After upgraded to the latest version of ts-bindings generated by soroban-cli 20.3.4, the signAndSend function always returns txTooLate error. The previous ts-bindings version works fine. (I'm interacting with the example increment contract deployed to testnet). @chadoh Could you please take a look? Thanks.

Here is the demo repo: https://github.com/shan-57blocks/soroban-ts-bindings-playground

Here is the error message:

Error SendFailedError: Sending the transaction to the network failed!
{
  "status": "ERROR",
  "hash": "f2f240838c0718e2e5fe9e156cee9588e7084262e1783f3d1866c2148a82db5a",
  "latestLedger": 1307318,
  "latestLedgerCloseTime": "1714111201",
  "errorResult": {
    "_attributes": {
      "feeCharged": {
        "_value": "1113253"
      },
      "result": {
        "_switch": {
          "name": "txTooLate",
          "value": -3
        }
      },
      "ext": {
        "_switch": 0
      }
    }
  }
}
    at _callee$ (/Users/57block/Documents/projects/huma-soroban-playground/node_modules/@huma/increment/node_modules/@stellar/stellar-sdk/lib/contract_client/sent_transaction.js:56:19)
    at tryCatch (/Users/57block/Documents/projects/huma-soroban-playground/node_modules/@huma/increment/node_modules/@stellar/stellar-sdk/lib/contract_client/sent_transaction.js:21:913)
    at Generator.<anonymous> (/Users/57block/Documents/projects/huma-soroban-playground/node_modules/@huma/increment/node_modules/@stellar/stellar-sdk/lib/contract_client/sent_transaction.js:21:2859)
    at Generator.next (/Users/57block/Documents/projects/huma-soroban-playground/node_modules/@huma/increment/node_modules/@stellar/stellar-sdk/lib/contract_client/sent_transaction.js:21:1550)
    at asyncGeneratorStep (/Users/57block/Documents/projects/huma-soroban-playground/node_modules/@huma/increment/node_modules/@stellar/stellar-sdk/lib/contract_client/sent_transaction.js:22:103)
    at _next (/Users/57block/Documents/projects/huma-soroban-playground/node_modules/@huma/increment/node_modules/@stellar/stellar-sdk/lib/contract_client/sent_transaction.js:23:194)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)

@chadoh
Copy link
Contributor Author

chadoh commented Apr 30, 2024

Does this mean that use of the bindings always downloads the contract spec in the user browser? Or is the contract spec embedded and the type generation is just dynamic?

The contract spec and the types are fetched at build time only. If you inspect a generated library, it now looks like this (using the hello_world contract):

export interface Client {
  /**
   * Construct and simulate a hello transaction. Returns an `AssembledTransaction` object which will have a `result` field containing the result of the simulation. If this transaction changes contract state, you will need to call `signAndSend()` on the returned object.
   */
  hello: ({to}: {to: string}, options?: {
    /**
     * The fee to pay for the transaction. Default: BASE_FEE
     */
    fee?: number;

    /**
     * The maximum amount of time to wait for the transaction to complete. Default: DEFAULT_TIMEOUT
     */
    timeoutInSeconds?: number;

    /**
     * Whether to automatically simulate the transaction when constructing the AssembledTransaction. Default: true
     */
    simulate?: boolean;
  }) => Promise<AssembledTransaction<Array<string>>>

}
export class Client extends ContractClient {
  constructor(public readonly options: ContractClientOptions) {
    super(
      new ContractSpec([ "AAAAAAAAAAAAAAAFaGVsbG8AAAAAAAABAAAAAAAAAAJ0bwAAAAAAEQAAAAEAAAPqAAAAEQ==" ]),
      options
    )
  }
  public readonly fromJSON = {
    hello: this.txFromJSON<Array<string>>
  }
}

Is this change a breaking change? Does the interface change that a developer interacts with? Saw one crew running into issues after updating to v20.3.4 saying that the interface was radically different.

Yes, it is a breaking change! And we didn't do enough testing on it, tbh 😞

Some fixes are rolling in, for ContractClient and related tools/repos like the frontend template used by the Getting Started tutorial and the soroban contract init command:

The updates to js-stellar-sdk haven't yet been released, and we haven't then made a subsequent update+release to the CLI for a new TS Bindings that relies on the updated stellar-sdk.

After upgraded to the latest version of ts-bindings generated by soroban-cli 20.3.4, the signAndSend function always returns txTooLate error. The previous ts-bindings version works fine. (I'm interacting with the example increment contract deployed to testnet). @chadoh Could you please take a look? Thanks.

I'm sorry you're running into this, @shan-57blocks. I'm not yet sure why the default timeout of 10 seconds wouldn't be enough for this; maybe the logic is off and the actual timeout is less than 10. I should have an answer on this later today. For now, you can try increasing the timeout. But! There's a bug in how you do this. This will fix part of the bug, once it's released and the CLI is updated to use it:

But for now, to increase the timeout, you can try this:

// @ts-ignore types generated in library do not match interface created in stellar-sdk!
const tx = await client.increment(undefined, { timeoutInSeconds: 30 })

Without the @ts-ignore, TypeScript will tell you that increment only takes one argument: the MethodOptions object. But the currently-released implementation of stellar-sdk always includes the arguments object first. If you do what TS tells you is correct—client.increment({ timeoutInSeconds: 30 })—then the actual implementation treats that object as an args object. I wish it would throw an error about that, but the way we parse it causes it to just be ignored, since increment doesn't expect any arguments.

@shan-57blocks
Copy link

@chadoh Just tested it with the below code and it worked:

// @ts-ignore types generated in library do not match interface created in stellar-sdk!
const tx = await client.increment(undefined, { timeoutInSeconds: 30 })

Looking forward to the new release.

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.

4 participants