-
Notifications
You must be signed in to change notification settings - Fork 315
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
optional simulate & wallet, editable TransactionBuilder #921
Conversation
* Fixup deprecation to specify exact version * Upgrade references to use latest modules
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>
* Add checks to ensure incorrect fields don't sneak in
* Upgrade all dependencies besides chai * Add changelog entries
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>
* Upgrade dependencies and stellar-base
- 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`.
5d637af
to
b22af74
Compare
src/soroban/assembled_transaction.ts
Outdated
/** | ||
* The maximum amount of time to wait for the transaction to complete. Default: {@link DEFAULT_TIMEOUT} | ||
*/ | ||
timeoutInSeconds?: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To move the discussion from here over:
Do you think this is important enough to bring to people's attention that we should uglify every method call in this way, or do you think there's a good-enough default?
Honestly? Yes. I'm open to opinions from users but from a security standpoint, this should be an explicit choice every time. Also from a consistency standpoint, you have to call setTimebounds
/setTimeout
on your TransactionBuilder
no matter what and intentionally opt-in to TimeoutInfinite
if you decide to make that (bad) choice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to make sure we're on the same page—this change removes the TimeoutInfinite
default. People would need to opt into that. The default is now ten seconds.
If you already understood that, sorry for over-explaining. If that's new information, does it change your opinion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh thanks for clarifying - I guess I missed that. I don't think there should be any default at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a huge change from current TS Bindings behavior and I'd like to hold off on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
works for me 👍 then a non-inf default is better
* Add valid response to not error in tests
* Add README entry about React-Native compiler requirements * Fixup the jsdoc for an entry * Upgrade stellar-base * Update all dependencies to their latest compatible versions
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.
🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎ To accept the risk, merge this PR and you will not be notified again.
Next stepsWhat is a non OSI license?(Experimental) Package has a non-OSI-approved license. Consider the terms of the license for your given use case. Take a deeper look at the dependencyTake 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 packageIf 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 riskTo ignore an alert, reply with a comment starting with
|
4959e4d
to
f757433
Compare
- 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
f757433
to
c1f3efd
Compare
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.
I don't feel comfortable merging as I'm not reviewing code as much as functionality. I'll leave that for @Shaptic |
* master: 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)
No need to pollute the global API or bundle size with this.
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.
@chadoh lmk if you'd rather I merge this and you resolve feedback in a later PR! doesn't matter to me |
@Shaptic ok, I addressed all your feedback here. I think this is ready to merge to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is turning into a really strong set of tools! Thanks for bearing with all of the back-and-forth on the design 👏
Thanks, @Shaptic! You'll have to merge, as I do not have permission. |
821dd35
to
ac7e891
Compare
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.
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`.
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).
* 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] <support@github.com> Co-authored-by: George <Shaptic@users.noreply.github.com> 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] <support@github.com> 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 <sergiocl@airtm.io> * 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] <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> * 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] <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>
Remove
TimeoutInfinite
, instead allowing people to set atimeoutInSeconds
when they initialize anAssembledTransaction
. Default: 10.No more
wallet
parameter. Instead, you pass:publicKey
– Will be used to construct theAccount
object that is used as the source of the transaction. Can be undefined, which means the null account will be used. Can be a string or, to ease operation with Freighter and other async-key-lookup in browser wallets, a promise of a string.signTransaction
: matches thesignTransaction
signature from Freighter. Can be left blank when you initialize the ContractClient and only provided at the time that you callsignAndSend({ signTransaction: (...) => { ... } })
signAuthEntry
: matches thesignAuthEntry
signature from Freighter. Can also be left initially blank, but is needed if/when you callsignAuthEntries
.You can now pass
simulate: false
when first creating your transaction to skip simulation. You can then modify the transaction using the TransactionBuilder attx.raw
before manually callingsimulate
. Example:Make
.simulation
getter publicError types are now collected under
AssembledTransaction.Errors
andSentTransaction.Errors
.Move
ContractClient
,AssembledTransaction
to separate entrypoints, so that they are not included in the global API or bundle size, but can still be packaged together with all the related stellar-sdk logic.Export
ExampleNodeWallet
from its own entrypoint, rather than hiding this logic entirely in the tests. @kalepail found this useful enough that he thought it would be worth exporting so other people can use it in their Node apps.