feat(sdk): integration with proving service - #480
noa-starkware wants to merge 1 commit into
Conversation
c3ef2de to
794ccf7
Compare
54fd897 to
9c7226b
Compare
9c7226b to
7b2c490
Compare
7b2c490 to
66f0ce8
Compare
| constructor( | ||
| private readonly params: { | ||
| account: Account; // the user account (for signing) | ||
| account: AccountSignerRaw; // the user account (for signing) |
There was a problem hiding this comment.
Can you use Account::sign_message instead? Using Account interface allows seamless wallet plug in here. Any additional interface wallets need to implement increase friction and maintenance burden
There was a problem hiding this comment.
Do you think this could work? The doc of sign_message: "Signs a JSON object for off-chain usage with the private key and returns the signature. This adds a message prefix so it can't be interchanged with transactions"
|
|
||
| async prove(invocation: ProofInvocationWithPayload): Promise<Proof> { | ||
| const inv = invocation; | ||
| const transactionPayload = { |
There was a problem hiding this comment.
Just curious why do you do this low level rather than using starknet.js helpers?
There was a problem hiding this comment.
Since this isnt a trivial call with an array of Calls, do you know of any helper I could use for this non conventional use case?
There was a problem hiding this comment.
This would probably do the trick:
import { type RpcProvider, ETransactionType } from "starknet";
const channel = (this.provider as RpcProvider).channel;
const transactionPayload = channel.buildTransaction(
{
type: ETransactionType.INVOKE,
contractAddress: invocation.contractAddress,
calldata: invocation.calldata,
signature: invocation.signature ?? [],
nonce: invocation.nonce,
resourceBounds: invocation.resourceBounds,
tip: invocation.tip,
paymasterData: invocation.paymasterData ?? [],
accountDeploymentData: invocation.accountDeploymentData ?? [],
nonceDataAvailabilityMode: invocation.nonceDataAvailabilityMode ?? "L1",
feeDataAvailabilityMode: invocation.feeDataAvailabilityMode ?? "L1",
},
"transaction"
);| * without actually generating zero-knowledge proofs. | ||
| */ | ||
| export class CallMockProofProvider implements ProofProviderInterface { | ||
| export class CallMockProofProvider extends AbstractProofProvider { |
There was a problem hiding this comment.
i'd suggest adding mocked tests to sdk as well to run in ci
There was a problem hiding this comment.
Working on mock prover for ci, is that what you meant?
| */ | ||
| protected async getViewingKey(): Promise<ViewingKey> { | ||
| return await this.viewingKeyProvider.getViewingKey(); | ||
| protected getViewingKey(): Promise<ViewingKey> | ViewingKey { |
| @@ -0,0 +1,55 @@ | |||
| /** | |||
| * Abstract base for proof providers with shared getDefaultDetails implementation. | |||
There was a problem hiding this comment.
Is this abstraction needed? Could we instead update ProofProviderInterface ?
There was a problem hiding this comment.
Both the mock and the real proof providers use the same get_details impl
There was a problem hiding this comment.
A simpler alternative would be a plain helper function (avoids extra abstraction layer):
// Replace abstract class with:
function getDefaultProofDetails(chainId: StarknetChainId): ProofInvocationFactoryDetails {
return { nonce: 0n, resourceBounds: { ... }, chainId, ... };
}
| | `VITE_ADMIN_ADDRESS` | Admin/minter account address | | ||
| | `VITE_ADMIN_KEY` | Admin account private key | | ||
| | `VITE_ACCOUNTS` | JSON array of user accounts (see `.env.example`) | | ||
| | `VITE_PROVING_SERVICE_URL` | Optional. Proving service URL (e.g. `http://136.115.124.93:3000`). If unset, the app uses the mock prover (`execute_view` only) | |
There was a problem hiding this comment.
I'd suggest adding support to the demo in a separate PR, this one should successfully run mocked tests in CI and e2e scenario against integration env
| type ProofInvocationFactoryDetails, | ||
| type ProofProviderInterface, | ||
| } from "starknet-sdk"; | ||
| import { IndexerClient } from "../src/indexer-client.js"; |
There was a problem hiding this comment.
You can use ContractDiscoveryProvider instead of indexer btw, to simplify your e2e flow (just test sdk<>proving service)
There was a problem hiding this comment.
I thought we wanted to test everything together, didnt we?
| @@ -5,16 +5,12 @@ | |||
| * MockServerAction[] callbacks as the proof output. | |||
There was a problem hiding this comment.
Note the signature validation error in CallMockProofProvider (see devnet tests)
de39ecd to
f805a3a
Compare
| } | ||
|
|
||
| /** Check result: proof non-empty, proof_facts and l2_to_l1_messages are arrays. */ | ||
| function isProveTransactionResult(value: unknown): value is ProveTransactionResult { |
There was a problem hiding this comment.
Do we need to validate proving service response?
| ); | ||
| } | ||
|
|
||
| export type BlockId = |
| // ============ Calldata ============ | ||
|
|
||
| /** Ensure calldata elements are 0x-prefixed hex (starknet signer/RPC expect this). */ | ||
| export function ensureHexCalldata(calldata: string[]): string[] { |
There was a problem hiding this comment.
It's used only once, perhaps worth just inlining?
| export interface ProofProviderInterface { | ||
| /** Get the default factory details for creating proof invocations */ | ||
| getDefaultDetails(): ProofInvocationFactoryDetails; | ||
| /** Get the default factory details for creating proof invocations (may be async e.g. to fetch nonce) */ |
There was a problem hiding this comment.
I'd make it always async to avoid mixing async/sync ret types
There was a problem hiding this comment.
Remove the comment, not relevant
There was a problem hiding this comment.
Note that two mixed return types remain:
- ViewingKeyProvider.getViewingKey(): returns Promise | ViewingKey
- ProofProviderInterface.prove(): returns Proof | Promise
| l2_to_l1_messages: MessageToL1[]; | ||
| } | ||
|
|
||
| export interface MessageToL1 { |
There was a problem hiding this comment.
With the same name?
There was a problem hiding this comment.
import type { RPC } from "starknet";
type MessageToL1 = RPC.RPCSPEC010.MSG_TO_L1;
There was a problem hiding this comment.
Although it's quite ugly :) Can probably keep the custom type
| /** | ||
| * Proving service URL. If a host is given (no scheme), defaults to http and port 3000. | ||
| */ | ||
| export function normalizeProvingServiceUrl(hostOrUrl: string): string { |
There was a problem hiding this comment.
Not sure if it's worth introducing a special helper for host + default port, just require users to provide a url in expected format
| } | ||
|
|
||
| /** Proof to bytes: u32[] packed big-endian, or base64 string decoded. */ | ||
| function proofToBytes(proof: number[] | string): Uint8Array { |
There was a problem hiding this comment.
Is proof returned as base64 string from the service? afaics it's serialized as list of integers https://github.com/starkware-libs/sequencer/blob/69b4c8d378580a1aec345f23dadd4766e036fe3b/crates/starknet_os_runner/src/server/http_server.rs#L35
| } | ||
|
|
||
| /** Map JSON-RPC error code to typed exception. */ | ||
| export function mapProvingServiceError(error: { |
There was a problem hiding this comment.
Separate classes for proving errors seems a bit of an overkill tbh, can just throw a detailed error message?
| senderAddress: poolAddressHex, | ||
| compiledCalldata, | ||
| version: (details.version ?? ETransactionVersion3.V3) as `${typeof ETransactionVersion3.V3}`, | ||
| nonce, |
There was a problem hiding this comment.
I'd suggest using existing methods from starknet.js for building/signing tx
There was a problem hiding this comment.
Because its non conventional invoke I dont know if its possible
629ec28 to
4a8e35a
Compare
1abc72b to
8742b39
Compare
| "node_modules/starknet": { | ||
| "version": "9.4.0", | ||
| "resolved": "git+ssh://git@github.com/m-kus/starknet.js.git#425082b072b705ac129d1898ea4fc9863fe367f9", | ||
| "resolved": "git+ssh://git@github.com/m-kus/starknet.js.git#19c1b5fcb227df60dc4d7e965cf1aea0699f543f", | ||
| "integrity": "sha512-26eJb/OY7nZA/bDGd8VTOwlp9NKBZsIjjYx1uqtr5ZRLiy2UsZQefWN5YfHiow1oyAzTO+dLtcfHUX410/TTSw==", | ||
| "license": "MIT", | ||
| "dependencies": { | ||
| "@noble/curves": "~1.7.0", |
There was a problem hiding this comment.
Package "$KEY" is resolved from a non-standard source: "git+ssh://git@github.com/m-kus/starknet.js.git#19c1b5fcb227df60dc4d7e965cf1aea0699f543f". Expected packages to be resolved from the official npm registry (registry.npmjs.org). Unusual sources may indicate dependency confusion, supply chain attacks, or misconfigured registries.
🌟 Fixed in commit 3b8850e 🌟
0bafa59 to
96f2d5b
Compare
20cf470 to
d5d2ec1
Compare
d5d2ec1 to
277d444
Compare
96f2d5b to
1092be3
Compare
277d444 to
faad3a9
Compare
1092be3 to
acf6e20
Compare
| // Mock proof provider returns proofFacts but proof.data may be undefined; use placeholder for sequencer | ||
| const response = await this.setup.admin.executeFromOutside(outsideTransaction, { | ||
| proofFacts: callAndProof.proofFacts, | ||
| proofFacts: callAndProof.proof.proofFacts, |
There was a problem hiding this comment.
Why isn't the proof passed ?
| fee_data_availability_mode: inv.feeDataAvailabilityMode ?? "L1", | ||
| }; | ||
|
|
||
| const result = await this.provingService.proveTransaction("latest", transactionPayload); |
There was a problem hiding this comment.
if we prove 'latest' only, it means now the wallet needs to wait 10 blocks before transmitting the tx. There should probably be some configuration that allows to specify 'latest' (in case we're proving a new state change) or 'latest-verifiable' or relative number from current block number (nice to have) or explicit block hash (for max reliability)
faad3a9 to
8a0efd1
Compare
b01d143 to
39bb920
Compare
8a0efd1 to
4d1b5d9
Compare
39bb920 to
bd07bf3
Compare
4d1b5d9 to
796b988
Compare
This change is