feat(web-client, react-sdk): contract creation, CompilerResource, transactions.execute(), and API improvements#1828
feat(web-client, react-sdk): contract creation, CompilerResource, transactions.execute(), and API improvements#1828WiktorStarczewski wants to merge 38 commits intonextfrom
Conversation
- Add ImmutableContract and MutableContract to AccountType freeze - Add newAccount() and addAccountSecretKeyToWebStore() as serialized wrappers on WebClient to prevent concurrent &mut self wasm-bindgen panics - Add accounts.create() dispatch for contract types via #createContract() which uses AccountBuilder + AuthSecretKey directly - Add accounts.getOrImport() convenience method - Widen accounts.import() fallback to accept full AccountRef (not just string)
Expose client.compile.component() and client.compile.txScript() on MidenClient.
Each call creates a fresh CodeBuilder so libraries from one compilation
never leak into another. txScript() accepts inline libraries as
{ namespace, code, linking? } objects where linking defaults to 'dynamic'
(suitable for FPI) or can be set to 'static' for off-chain libraries.
Wraps TransactionRequestBuilder + optional foreignAccounts + submit into
a single call. Accepts a compiled TransactionScript from compile.txScript().
foreignAccounts accepts either bare AccountRef values or { id, storage? }
wrapper objects. The wrapper vs WASM discrimination uses
'id' in fa && typeof fa.id !== 'function' to distinguish plain objects
from WASM AccountId/Account instances (which expose id() as a method).
- Import StorageSlot, AccountComponent, AuthSecretKey, AccountStorageRequirements, TransactionScript from WASM types - Extend AccountType and AccountTypeValue with ImmutableContract/MutableContract - Add ContractCreateOptions interface and include it in CreateAccountOptions - Widen ImportAccountInput from string to AccountRef - Add CompilerResource, CompileComponentOptions, CompileTxScriptLibrary, CompileTxScriptOptions interfaces - Add ExecuteOptions interface extending TransactionOptions - Add getOrImport() to AccountsResource, execute() to TransactionsResource - Add readonly compile: CompilerResource to MidenClient class
13 tests across 4 describe blocks:
- compile.component(): procedure hashes, withSupportsAllTypes, builder isolation
- compile.txScript(): without libraries, with dynamic library, builder isolation
- accounts.create() contracts: ImmutableContract/MutableContract props,
default public storage, no-component creation, deterministic ID from seed
- transactions.execute(): returns txId, increments storage, foreignAccounts
as { id } wrapper and as bare AccountId
…execute() - new-accounts.md: add 'Creating a Custom Contract Account' section showing compile.component() + accounts.create() with ImmutableContract type - compile.md (new): full guide for client.compile — component(), txScript(), linking modes (dynamic vs static), and getProcedureHash() for FPI - new-transactions.md: replace 'Custom Transactions' with transactions.execute() and an FPI subsection; retain raw TransactionRequestBuilder as 'Advanced' - accounts.md: add getOrImport() documentation - import.md: widen 'Importing by Account ID' to show all AccountRef forms (hex, bech32, AccountId, Account); add tip pointing to getOrImport()
…NoteId, InputNoteRecord)
… via AccountRef type
The local resolveAccountId helpers in useImportAccount and useTransaction only accepted string | AccountId, but options.accountId is typed as AccountRef (which includes Account | AccountHeader). Updated both to accept AccountRef, matching the parseAccountId signature they delegate to. Also regenerated typedoc web-client documentation to match current types.
- Harden accounts.import() dispatch with early exit for AccountRef types - Replace minification-unsafe constructor.name checks with duck-typing - Add error handling to #createContract for partial-state failures - Add assertNotTerminated() guard to getOrImport - Add "network" to ContractCreateOptions.storage type - Add defensive getWasm() call in compiler.txScript() - Add input validation for contract creation (seed, auth) - Add inline comments for foreign account wrapper detection - Document getProcedureHash first-match ambiguity in Rust doc comment
- "no extra components" test: changed to verify the expected error (protocol requires at least one non-auth procedure for contracts) - "same seed yields same account ID" test: use raw AccountBuilder for the second build to avoid IndexedDB duplicate-key error (all mock clients share the same "mock_client_db" database) - Two foreignAccounts tests: marked as test.fixme() because the mock chain does not track SDK-created accounts in its committed list, causing get_account_proof to panic on lookup
…e useInternalTransfer
- Add `authenticated: false` option to `send()` — creates a P2ID note locally
via `Note.createP2IDNote` + `withOwnOutputNotes`, returns `{ txId, note }`
- Both send paths now return `{ txId, note }` (note is null for authenticated)
- Update `#buildConsumeRequest` to detect direct Note objects via duck-typing
and use `TransactionRequestBuilder.withInputNotes(NoteAndArgsArray)` for
unauthenticated notes not registered in the store
- Add `SendOptionsAuthenticated`, `SendOptionsUnauthenticated`, `SendResult` types
with overloaded `send()` signatures in api-types.d.ts
- React SDK: extend `useSend` with unauthenticated path returning `SendResult`
- React SDK: extend `useConsume` to accept `Note` objects in `noteIds` directly
- React SDK: remove `useInternalTransfer` hook (replaced by `send({ authenticated: false })` + `consume()` loop)
- React SDK: add `SendResult`, `Note` to public exports; remove `InternalTransfer*` types
Accept 'local', 'devnet', 'testnet', or a raw URL string for proverUrl. 'local' maps to newLocalProver(); network shorthands map to known remote URLs.
Accept 'testnet', 'devnet', 'localhost'/'local' or a raw URL string for rpcUrl. Mirrors the React SDK's resolveRpcUrl behavior.
igamigo
requested review from
JereSalo and
juan518munoz
and removed request for
juan518munoz
| readonly ImmutableContract: "ImmutableContract"; | ||
| readonly MutableContract: "MutableContract"; |
There was a problem hiding this comment.
What is a MutableContract/ImmutableContract and how are they different to MutableWallet/ImmutableWallet? Is it just that the wallets contain the specific components? In general, I think "contract" is not used too much (usually it's "mutable code"), but maybe this is something worth revising
There was a problem hiding this comment.
Yeah "wallets" need no seed, no auth, no components, whereas
// Wallet: no seed, no auth, no components required
const wallet = await client.accounts.create();
// Contract: everything must be provided
const contract = await client.accounts.create({
type: "ImmutableContract",
seed,
auth,
components: [myComponent],
});
I thought long and hard how else to call it, but Contract just fits so well from the SDK/developer standpoint, it's exactly the right abstraction. And while we don't use it at the protocol level, the closer we get to usage, the more likely we are to start talking contracts; case in point, the Tutorial is literally called "Deploying a Counter Contract". Happy to brainstorm some alternatives though if you have any...
| export interface SendOptionsUnauthenticated extends TransactionOptions { | ||
| account: AccountRef; | ||
| to: AccountRef; | ||
| token: AccountRef; | ||
| amount: number | bigint; | ||
| type?: NoteVisibility; | ||
| authenticated: false; | ||
| } |
There was a problem hiding this comment.
Why is this called "unauthenticated"? Usually unauthenticated/authenticated notes are a property of notes in terms of how they are being consumed, not created
There was a problem hiding this comment.
Fair, I can see how the naming can be confusing. I refactored to returnNote: true (false being the default obviously). Does this sound better? The point of this is to keep the send() flow, which in this case returns the note itself.
| ## Get or Import an Account | ||
|
|
||
| `getOrImport()` returns the locally stored account if it exists, or imports it from the network if it does not. This is a convenience wrapper around `get()` + `import()` and is the recommended way to load a public account you may or may not already have locally: |
There was a problem hiding this comment.
What would this be used for? This will import the account into the client and as part of the sync process you would receive all of that account's updates
There was a problem hiding this comment.
Yeah it's true if you import an account, sync will handle updates automatically. But getOrImport is a convenience for the common pattern of "I have an account ID, I want to interact with it, I don't know if it's local yet." Without it, devs would write try { get() } catch { import() } everywhere. That said, the docs could better explain when you'd use this vs. just relying on sync. I've improved the docs.
| /// Returns the hex-encoded MAST root for a procedure by name. | ||
| /// | ||
| /// Matches by full path, relative path, or local name (after the last `::`). | ||
| /// When matching by local name, if multiple procedures share the same local | ||
| /// name across modules, the first match is returned. | ||
| #[wasm_bindgen(js_name = "getProcedureHash")] | ||
| pub fn get_procedure_hash(&self, procedure_name: &str) -> Result<String, JsValue> { | ||
| let library = self.0.component_code().as_library(); | ||
|
|
||
| let get_proc_export = library | ||
| .exports() | ||
| .find(|export| { | ||
| export.as_procedure().is_some() | ||
| && (export.path().as_ref().as_str() == procedure_name | ||
| || export.path().as_ref().to_relative().as_str() == procedure_name) | ||
| if export.as_procedure().is_none() { | ||
| return false; | ||
| } | ||
| let export_path = export.path(); | ||
| let path_str = export_path.as_ref().as_str(); | ||
| path_str == procedure_name | ||
| || export_path.as_ref().to_relative().as_str() == procedure_name | ||
| || path_str.rsplit_once("::").is_some_and(|(_, local)| local == procedure_name) | ||
| }) | ||
| .ok_or_else(|| { | ||
| JsValue::from_str(&format!( |
There was a problem hiding this comment.
Instead of this, if something more general than "get procedure hash by name" is needed, would adding a way of getting all procedure names with their hashes work? Feels like it would be more convenient than having an opinionated way of parsing and returning
There was a problem hiding this comment.
I really like the getAllProcedures idea, however I'm not sure this is a replacement. The main point here is the fuzzy matching (especially the rsplit_once("::") fallback) - which is convenient, because from an SDK ergonomics perspective, the fuzzy matching is what JS devs actually want. They don't want to type
#miden::contracts::counter::increment_count, they want to say "increment_count". Making them call getAllProcedures() and filter manually is worse DX for the common case IMO.
There was a problem hiding this comment.
There have to be some conceptual differences between the WebClient JS bindings and the overall higher level API though, no? In that sense, I wonder if it's a better idea to move the fuzzy matching to the higher level MidenClient (or elsewhere), and leave the bindings to more properly express the functionalities of the underlying client/protocol crates. Just a thought though, no strong opinions.
| // get_account_proof panics when resolving foreign accounts. This test | ||
| // requires a real node or an enhanced mock that registers SDK-created | ||
| // accounts in MockChain.committed_accounts. | ||
| test.fixme("execute() with foreignAccounts accepts a plain { id } wrapper", async ({ |
There was a problem hiding this comment.
Doesn't this mean that this test is broken?
There was a problem hiding this comment.
No, the tests are test.fixme(). This is Playwright's way of marking known-failing tests that are skipped. They're documented with clear explanations of why they're disabled (mock chain doesn't track SDK-created accounts). This is the pattern for "test we want but can't run yet.". I can remove if it leaves a sour taste but I like the fixme marking for things I plan to revisit (especially if like here, it turns out it doesn't work after writing it ;) and you're faced with either removing the code or fixing the underlying issue and neither sounds like a good idea). Not a big deal though.
|
|
||
| // Same mock limitation as above: SDK-created accounts are not in the mock | ||
| // chain's committed list, causing get_account_proof to panic. | ||
| test.fixme("execute() with a bare AccountId as foreignAccount (non-wrapper path)", async ({ |
- listAvailable() now returns InputNoteRecord[] directly instead of ConsumableNoteRecord[], removing the need to call .inputNoteRecord() - useWaitForNotes returns InputNoteRecord[] instead of ConsumableNoteRecord[] - Rename ConsumeOptions.noteIds to ConsumeOptions.notes, accepting strings, NoteId, InputNoteRecord, or Note objects directly - Remove ConsumableNoteRecord from public API exports
…returnNote The `authenticated` flag on send() was misusing Miden protocol terminology. In Miden, authenticated/unauthenticated refers to note consumption, not creation. The flag actually controls whether the Note object is built in JS and returned to the caller. Rename to `returnNote` which accurately describes the behavior. - SendOptionsAuthenticated -> SendOptionsDefault - SendOptionsUnauthenticated -> SendOptionsReturnNote - authenticated: false -> returnNote: true
…NoteRecord typedoc
WiktorStarczewski
changed the title
WiktorStarczewski
changed the title
| * [FEATURE][web] New `MidenClient` class with resource-based API (`client.accounts`, `client.transactions`, `client.notes`, `client.tags`, `client.settings`). Provides high-level transaction helpers (`send`, `mint`, `consume`, `swap`, `consumeAll`), transaction dry-runs via `preview()`, confirmation polling via `waitFor()`, and flexible account/note references that accept hex strings, bech32 strings, or WASM objects interchangeably (`AccountRef`, `NoteInput` types). Factory methods: `MidenClient.create()`, `MidenClient.createTestnet()`, `MidenClient.createMock()`. ([#1762](https://github.com/0xMiden/miden-client/pull/1762)) | ||
| * [FEATURE][web] Added `TransactionId.fromHex()` static constructor for creating transaction IDs from hex strings. ([#1762](https://github.com/0xMiden/miden-client/pull/1762)) | ||
| * [FEATURE][web] Added standalone tree-shakeable note utilities (`createP2IDNote`, `createP2IDENote`, `buildSwapTag`) usable without a client instance. ([#1762](https://github.com/0xMiden/miden-client/pull/1762)) | ||
| * [FEATURE][web] Custom contract support: `accounts.create()` with `ImmutableContract`/`MutableContract` types, new `client.compile` resource (`compile.component()`, `compile.txScript()` with `"dynamic"`/`"static"` linking), and `transactions.execute({ account, script, foreignAccounts? })` for custom script execution with FPI. ([#1828](https://github.com/0xMiden/miden-client/pull/1828)) |
There was a problem hiding this comment.
client.trnsaction.send({...}) changed its return type so this changes should be marked as [BREAKING] too.
| return wasm.AccountComponent.compile( | ||
| compiled, | ||
| slots | ||
| ).withSupportsAllTypes(); |
There was a problem hiding this comment.
should this be opt-out?
|
This PR addresses a real issue in the high-level API and the core feature is well designed. However, reviewing it thoroughly is difficult because it bundles at least 5 different feature areas into a single PR (38 commits, +2449/-795, 66 files). Beyond the scope of the original issue, the PR also includes:
Each of these unrelated changes is a reasonable change on its own, but grouping them makes it hard to review each feature correctness, identify breaking changes and revert regressions if something goes wrong after merge. Would it be possible to split this into the core feature as one PR, and the rest as follow-ups? At minimum, the breaking changes in item 1 (send return type + useInternalTransfer removal) would benefit from being separate so they get proper visibility. |
JereSalo
left a comment
JereSalo
left a comment
There was a problem hiding this comment.
Nice work. These are some notes that I've taken while reviewing this PR, some of them are just nits and could even be ignored:
-
The
send()withreturnNote === truehas zero test coverage, we should probably add tests for it. -
Some fields still use
stringwhere they could acceptAccountRef. For example,MintOptionsstill typestargetAccountIdandfaucetIdasstring, anduseAccountacceptsstring | AccountIdinstead ofAccountRef. I just thought it was worth noting though we can change it afterwards as it may be out of scope. -
We changed
stringtoAccountRefin many places but kept the field nameaccountId. Should we rename those to something likeaccountRefto reflect the broader accepted type, or keepaccountIdas-is? -
resolveAccountIdis defined in two hooks and is just a one-line wrapper aroundparseAccountId. Is there a reason not to remove it and callparseAccountIddirectly?
4 participants
Summary
This PR adds custom smart contract support to the web client SDK and makes several API ergonomics improvements across both the web client and React SDK.
Core features
accounts.create()supportsImmutableContractandMutableContracttypes with user-provided compiled componentsclient.compilewithcompile.component({ code, slots })andcompile.txScript({ code, libraries? })supporting"dynamic"/"static"library linkingtransactions.execute({ account, script, foreignAccounts? })for running arbitrary transaction scripts with foreign procedure invocation (FPI)Send API rework
send()now returns{ txId, note }(SendResult) instead of bareTransactionIdreturnNote: trueoption builds the P2ID note in JS and returns theNoteobject (for out-of-band delivery to recipient)SendOptionsDefaultandSendOptionsReturnNotediscriminated union typesNotes API simplification
notes.listAvailable()returnsInputNoteRecord[]directly (removedConsumableNoteRecordwrapper)consume()now acceptsNoteobjects directly (not just IDs) — enables consuming notes fromsend({ returnNote: true })without persisting to store firstuseInternalTransferhook — replaced byuseSend({ returnNote: true })+useConsume({ notes: [note] })SDK ergonomics
AccountId,NoteId,TransactionId,Account,AccountHeader,InputNoteRecord) via duck-typing — no manual.toHex()/.toString()neededAccountReftype in React SDK (acceptsstring | AccountId | Account | AccountHeaderin all hooks)NoteVisibilityandStorageModeenum constants (replaces magic strings)MidenClient.create()acceptsrpcUrl/provershorthands ("testnet","devnet","localhost","local")accounts.getOrImport()convenience methodaccounts.import()widened to acceptAccountRefRust changes
getProcedureHash()now matches by local name (after last::) in addition to full/relative pathDocumentation
getOrImportguidance), account import (AccountRefflexibility)Breaking changes
send()returns{ txId, note }notTransactionIdresult.txIdnotes.listAvailable()returnsInputNoteRecord[].inputNoteRecord()callsConsumableNoteRecordremoved from exportsInputNoteRecorddirectlyConsumeOptions.noteIds→notesuseSendreturnsSendResult(txId+note).transactionIdto.txIduseInternalTransferremoveduseSend({ returnNote: true })+useConsume()Test plan