feat(proposals): Support custom proposals#268
Open
zeljkoX wants to merge 4 commits into
Open
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis pull request implements a symmetric "custom proposal producer" API across Rust and TypeScript Miden multisig SDKs, enabling integrations to create and execute arbitrary multisig proposal types with integration-owned transaction recipes while the SDK enforces binding checks and commitment validation. ChangesCustom Proposal Producer API
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR extends Guardian + the Rust/TypeScript multisig SDKs to support arbitrary (server-defined) proposal_type labels by bucketing unmodeled types as custom while preserving the original label for display, and introduces a producer-side “bring-your-own-transaction” execution-prep flow for custom proposals.
Changes:
- Remove the server-side allowlist for
metadata.proposal_type(now any non-empty string is accepted) and add regression tests. - Update SDK typing/metadata codecs to preserve unmodeled proposal labels as
custom+rawProposalType(TS) /TransactionType::Custom+proposal_type(Rust). - Add producer APIs + harness/docs/examples for proposing custom artifacts and preparing execution advice (cosigner sigs + GUARDIAN ack) without the SDK building/submitting the custom transaction itself.
Reviewed changes
Copilot reviewed 37 out of 39 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| speckit/features/008-custom-proposal-producer/spec.md | New feature spec for custom proposal producer API and execution-prep flow. |
| speckit/features/008-custom-proposal-producer/data-model.md | SDK-level data model and invariants for custom artifacts/labels/binding. |
| speckit/features/008-custom-proposal-producer/contracts/sdk-api.md | Rust/TS SDK surface + behavioral/error contract for producer APIs. |
| spec/api.md | Document server acceptance of arbitrary proposal_type labels. |
| packages/miden-multisig-client/src/types/proposal.ts | Define closed modeled proposal types plus custom; add rawProposalType. |
| packages/miden-multisig-client/src/proposal/metadata.ts | Round-trip arbitrary labels as custom while preserving raw label. |
| packages/miden-multisig-client/src/proposal/metadata.test.ts | Add tests for custom/raw-label codec behavior. |
| packages/miden-multisig-client/src/multisig/proposal/parser.ts | Remove legacy parser implementation (now handled elsewhere). |
| packages/miden-multisig-client/src/multisig/proposal/execution.ts | Block generic execution for custom proposals with a clearer error. |
| packages/miden-multisig-client/src/multisig.ts | Add proposeCustom, prepareCustomExecution, and submitTransaction. |
| packages/miden-multisig-client/README.md | Document custom proposal types + producer workflow in TS SDK. |
| packages/guardian-client/src/types.ts | Allow arbitrary proposalType strings in the wire contract. |
| packages/guardian-client/src/server-types.ts | Allow arbitrary proposal_type strings in server wire types. |
| packages/guardian-client/src/conversion.ts | Preserve arbitrary proposal_type when converting to server payload. |
| packages/guardian-client/src/conversion.test.ts | Add coverage that arbitrary proposal_type is preserved. |
| examples/smoke-web/src/smokeHarness.ts | Add create/execute custom proposal commands and in-session recipe cache. |
| examples/smoke-web/src/App.tsx | UI controls for proposing/executing a custom (producer) proposal. |
| examples/smoke-web/README.md | Document smoke-web custom proposal producer flow usage. |
| examples/demo/src/state.rs | Cache producer recipes in demo session state for custom execution. |
| examples/demo/src/actions/proposal_management.rs | Add CLI actions to create/execute custom proposals in Rust demo. |
| examples/_shared/multisig-browser/src/multisigApi.ts | Add shared helper APIs for custom proposal recipe + prepare/submit. |
| examples/_shared/multisig-browser/package.json | Bump @miden-sdk/miden-sdk to 0.14.5. |
| docs/MULTISIG_SDK.md | Document custom proposal bucket + symmetric producer API behavior. |
| crates/server/src/services/push_delta_proposal.rs | Add server test ensuring custom proposal_type is accepted. |
| crates/server/src/services/mod.rs | Remove proposal_type allowlist; reject only empty/whitespace labels. |
| crates/miden-multisig-client/src/transaction/mod.rs | Add artifact deserializer helper + tests for garbage/empty bytes. |
| crates/miden-multisig-client/src/transaction/builder.rs | Reject TransactionType::Custom in typed proposal builder path. |
| crates/miden-multisig-client/src/proposal.rs | Add TransactionType::Custom and built-in label detection helper. |
| crates/miden-multisig-client/src/payload.rs | Add payload helper to set custom metadata proposal_type. |
| crates/miden-multisig-client/src/lib.rs | Re-export producer helpers (deserialize/build/salt/signature advice). |
| crates/miden-multisig-client/src/export.rs | Ensure export/import preserves custom proposal_type label. |
| crates/miden-multisig-client/src/execution.rs | Prevent SDK from building final tx request for custom proposals. |
| crates/miden-multisig-client/src/error.rs | Add UnsupportedTransactionType for custom/unmodeled operations. |
| crates/miden-multisig-client/src/client/proposals.rs | Implement propose_custom, prepare_custom_execution, submit_transaction. |
| crates/miden-multisig-client/src/client/helpers.rs | Skip metadata reconstruction checks for opaque custom proposals. |
| crates/miden-multisig-client/src/account.rs | Treat custom threshold as default account threshold. |
| crates/miden-multisig-client/README.md | Document custom producer flow in Rust SDK. |
| .gitignore | Ignore .DS_Store and *.artifact. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/miden-multisig-client/src/types/proposal.ts (1)
92-106:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake
rawProposalTyperequired in the domain model.This currently allows
{ proposalType: 'custom' }even though the original server label is what lets custom proposals round-trip back to GUARDIAN/export correctly. If backward compatibility is needed, keep that optionality in the transport/parser layer and convert immediately into a required domain shape.💡 Proposed change
export interface CustomProposalMetadata extends BaseProposalMetadata { proposalType: 'custom'; /** Original server-defined proposal label, e.g. "b2agg" (issue `#266`). Mirrors * Rust `ProposalMetadata.proposal_type` so both SDKs expose the real type. - * Optional for backward compatibility; the parser always populates it. */ - rawProposalType?: string; + * Always required in the domain model. */ + rawProposalType: string; }As per coding guidelines, "Prefer mandatory fields in domain and request-facing types over optional ones" and "Do not add backward-compatibility layers, migrations, or fallback behavior unless the task explicitly requires them".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/miden-multisig-client/src/types/proposal.ts` around lines 92 - 106, Make rawProposalType required on the domain model by changing CustomProposalMetadata.rawProposalType from optional to mandatory (remove the `?`), so CustomProposalMetadata always has a server label; update any construction/parsing code that produces domain CustomProposalMetadata (e.g., the parser/transport layer that currently accepts `{ proposalType: 'custom' }`) to accept the optional transport field but immediately populate and validate rawProposalType before creating the domain CustomProposalMetadata; keep the transport/parser layer optional if needed but ensure conversion/validation happens where instances of ProposalMetadata (the union including CustomProposalMetadata) are composed.crates/server/src/services/mod.rs (1)
227-241:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPersist the trimmed
proposal_type, not the raw input.
" p2id "and" b2agg "both pass the current check, but you write the untrimmed value back into metadata. That makes padded built-in labels slip past the reserved-name contract and creates distinct server labels for the same logical proposal type.🐛 Proposed change
let proposal_type = obj .get("proposal_type") .and_then(Value::as_str) .ok_or_else(|| { GuardianError::InvalidDelta("metadata.proposal_type is required".to_string()) })?; - if proposal_type.trim().is_empty() { + let proposal_type = proposal_type.trim(); + if proposal_type.is_empty() { return Err(GuardianError::InvalidDelta( "metadata.proposal_type must not be empty".to_string(), )); } obj.insert( "proposal_type".to_string(), Value::String(proposal_type.to_string()), );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/server/src/services/mod.rs` around lines 227 - 241, The code validates proposal_type by trimming for emptiness but then writes the original untrimmed string back; change it to persist the trimmed value: after obtaining proposal_type (from Value::as_str), call let proposal_type = proposal_type.trim(); use that trimmed value for the empty check and for obj.insert (i.e., write Value::String(proposal_type.to_string())), ensuring any leading/trailing whitespace is removed before persisting (references: the proposal_type variable, the empty check, and the obj.insert(Value::String(...)) call).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/miden-multisig-client/src/account.rs`:
- Line 146: The test
effective_threshold_for_transaction_maps_to_expected_procedures() needs to treat
TransactionType::Custom like the account default: update the test so that when
iterating transaction types you include TransactionType::Custom and assert it
maps to the account.default threshold (or call
effective_threshold_for_transaction() and expect self.threshold()), and
add/adjust a focused unit test that covers the custom-threshold branch in
effective_threshold_for_transaction() to ensure TransactionType::Custom returns
the account default; locate references to effective_threshold_for_transaction(),
TransactionType::Custom, and
effective_threshold_for_transaction_maps_to_expected_procedures() in the test
module and modify/assert accordingly.
In `@crates/miden-multisig-client/src/client/helpers.rs`:
- Around line 197-202: The current early return when
matches!(proposal.transaction_type, TransactionType::Custom) skips validation of
transport metadata and lets malformed GUARDIAN payloads mark custom proposals
ready; instead, remove the unconditional return and validate custom proposal
metadata at the boundary in the same helper that handles other types:
parse/normalize the transport metadata into required domain types on proposal
(reconciling any optional protobuf fields), and at minimum verify the custom
proposal's required signature count against the current account threshold (use
the same account/threshold lookup logic used elsewhere) before allowing
proposal.status.is_ready() to be trusted; ensure prepare_custom_execution() is
only reachable after these checks.
In `@crates/miden-multisig-client/src/client/proposals.rs`:
- Around line 405-417: The submit_transaction path uses the SDK's cached state
and can fail if the account advanced after prepare_custom_execution; before
calling miden_client.submit_new_transaction in submit_transaction, call the
client's sync method (e.g., self.sync().await? or await
self.miden_client.sync_state()) to refresh local state, then proceed to
require_account() and submit_new_transaction; update error handling to preserve
the existing MultisigError mapping and keep the subsequent sync_state() call if
still needed.
In `@crates/miden-multisig-client/src/error.rs`:
- Around line 91-94: Add a stable error code for the new public variant
UnsupportedTransactionType and return it from MultisigError::code(): update the
enum variant metadata/implementation so it maps to a constant stable string
(e.g. "unsupported_transaction_type") and modify the MultisigError::code() match
to return Some("...") for UnsupportedTransactionType; ensure the returned code
is used consistently with other variants and documented in the same style as
existing codes so callers can rely on a stable identifier instead of parsing the
message.
In `@examples/_shared/multisig-browser/src/multisigApi.ts`:
- Around line 446-459: The current catch block treats any account nonce increase
as proof submitTransaction succeeded, which can be caused by unrelated
proposals; instead, after await multisig.syncState() verify that the specific
proposal represented by finalRequest was actually applied (e.g., look up the
transaction/proposal by its id, txHash, or unique fields in the multisig's
on-chain/ synced state via a method like multisig.getTransaction /
multisig.findTransaction / multisig.transactions and confirm it matches
finalRequest and is executed); only swallow submitError if that specific
proposal is found executed and otherwise re-throw submitError.
In `@examples/demo/src/state.rs`:
- Around line 156-163: Both cache_custom_recipe and get_custom_recipe only trim
a lowercase "0x" and miss IDs starting with "0X"; normalize the ID case before
trimming so prefixes are handled case-insensitively. Update both functions
(cache_custom_recipe and get_custom_recipe) to first convert proposal_id to
lowercase (e.g., let id = proposal_id.to_lowercase()) and then call
trim_start_matches("0x") on that normalized string before inserting or looking
up in self.custom_recipes.
In `@examples/smoke-web/src/App.tsx`:
- Around line 568-584: The custom-execute button should only be enabled or shown
when the current session actually has the producer-owned recipe required by
executeCustomProposal; update the JSX branch that checks
proposal.metadata.proposalType === 'custom' to also verify the session's recipe
availability (e.g. a boolean like sessionHasRecipe or a helper such as
recipeAvailableForProposal(proposal)) and if missing either disable the button
(disabled={!sessionReady || !sessionHasRecipe}) and change the label to indicate
"Missing recipe" or hide the custom button entirely, wiring the onClick to
handleExecuteCustomProposal(proposal.id) only when the recipe exists; keep the
non-custom branch using runAction(() => api.executeProposal({ proposalId:
proposal.id })) unchanged.
In `@examples/smoke-web/src/smokeHarness.ts`:
- Around line 374-375: customRecipesRef (a Map stored in useRef) persists across
account changes and can return recipes from a previous session; to fix, clear or
reinitialize it whenever account-scoped state is reset — add
customRecipesRef.current.clear() (or customRecipesRef.current = new Map())
inside the routines that reset account state such as createAccount, loadAccount,
and clearLocalState so executeCustomProposal({ proposalId }) cannot resolve
recipes from the prior account/session.
- Around line 1149-1159: If both input.recipe and input.proposalId are provided,
validate they match (compare trimmed input.proposalId to recipe.proposalId) and
throw a clear error if they differ; additionally verify the resolved
recipe.senderId equals the currently loaded multisig identity (currentMultisig
or its id) and throw if it does not, to prevent using a recipe from another
account; use the correct key when deleting from customRecipesRef.current (use
the validated proposalId) and only call prepareAndSubmitCustomProposal after
these checks succeed.
In `@packages/guardian-client/src/conversion.test.ts`:
- Around line 136-145: The test only validates fromServerProposalMetadata; add
the reverse assertion to ensure the camelCase→server mapping preserves custom
labels by calling toServerProposalMetadata({ proposalType: 'b2agg' }) and
asserting the returned object has proposal_type === 'b2agg'. Update the test in
conversion.test.ts to include this extra expect so regressions in
toServerProposalMetadata (the mapping code in conversion.ts) are caught.
In `@packages/miden-multisig-client/src/multisig.ts`:
- Around line 101-109: BUILTIN_PROPOSAL_TYPES is missing the reserved label
'custom', allowing proposeCustom() to accept a user-defined proposal that
conflicts with the SDK's internal bucket name; add the string 'custom' to the
BUILTIN_PROPOSAL_TYPES Set so proposeCustom() will reject that reserved label
and maintain the raw-label vs. bucket separation enforced by
CustomProposalMetadata.
- Around line 1639-1642: Reject custom proposals early in
prepareProposalExecution by checking metadata.proposalType === 'custom' and
throwing before any advice assembly or calls to this.guardian.pushDelta(...);
specifically, move or add the guard at the start of prepareProposalExecution (or
the entry point used by executeProposal/createTransactionProposalRequest) so
buildTransactionRequestFromMetadata and guardian.pushDelta are never invoked for
'custom' proposals, keeping the flow side-effect free.
In `@packages/miden-multisig-client/src/multisig/proposal/execution.ts`:
- Around line 359-363: Guard against custom proposals earlier by checking
proposal.metadata.proposalType === 'custom' before any side-effecting calls like
appendGuardianAckAdvice() or guardian.pushDelta; move or add the validation in
the exported workflows (the functions that call appendGuardianAckAdvice()) so
they throw the same Error for custom proposals immediately and never call
appendGuardianAckAdvice(), ensuring guardian.pushDelta is not invoked for
unsupported custom types.
---
Outside diff comments:
In `@crates/server/src/services/mod.rs`:
- Around line 227-241: The code validates proposal_type by trimming for
emptiness but then writes the original untrimmed string back; change it to
persist the trimmed value: after obtaining proposal_type (from Value::as_str),
call let proposal_type = proposal_type.trim(); use that trimmed value for the
empty check and for obj.insert (i.e., write
Value::String(proposal_type.to_string())), ensuring any leading/trailing
whitespace is removed before persisting (references: the proposal_type variable,
the empty check, and the obj.insert(Value::String(...)) call).
In `@packages/miden-multisig-client/src/types/proposal.ts`:
- Around line 92-106: Make rawProposalType required on the domain model by
changing CustomProposalMetadata.rawProposalType from optional to mandatory
(remove the `?`), so CustomProposalMetadata always has a server label; update
any construction/parsing code that produces domain CustomProposalMetadata (e.g.,
the parser/transport layer that currently accepts `{ proposalType: 'custom' }`)
to accept the optional transport field but immediately populate and validate
rawProposalType before creating the domain CustomProposalMetadata; keep the
transport/parser layer optional if needed but ensure conversion/validation
happens where instances of ProposalMetadata (the union including
CustomProposalMetadata) are composed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9702e687-6894-4988-91f2-763238c3fee0
⛔ Files ignored due to path filters (1)
examples/_shared/multisig-browser/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (38)
.gitignorecrates/miden-multisig-client/README.mdcrates/miden-multisig-client/src/account.rscrates/miden-multisig-client/src/client/helpers.rscrates/miden-multisig-client/src/client/proposals.rscrates/miden-multisig-client/src/error.rscrates/miden-multisig-client/src/execution.rscrates/miden-multisig-client/src/export.rscrates/miden-multisig-client/src/lib.rscrates/miden-multisig-client/src/payload.rscrates/miden-multisig-client/src/proposal.rscrates/miden-multisig-client/src/transaction/builder.rscrates/miden-multisig-client/src/transaction/mod.rscrates/server/src/services/mod.rscrates/server/src/services/push_delta_proposal.rsdocs/MULTISIG_SDK.mdexamples/_shared/multisig-browser/package.jsonexamples/_shared/multisig-browser/src/multisigApi.tsexamples/demo/src/actions/proposal_management.rsexamples/demo/src/state.rsexamples/smoke-web/README.mdexamples/smoke-web/src/App.tsxexamples/smoke-web/src/smokeHarness.tspackages/guardian-client/src/conversion.test.tspackages/guardian-client/src/conversion.tspackages/guardian-client/src/server-types.tspackages/guardian-client/src/types.tspackages/miden-multisig-client/README.mdpackages/miden-multisig-client/src/multisig.tspackages/miden-multisig-client/src/multisig/proposal/execution.tspackages/miden-multisig-client/src/multisig/proposal/parser.tspackages/miden-multisig-client/src/proposal/metadata.test.tspackages/miden-multisig-client/src/proposal/metadata.tspackages/miden-multisig-client/src/types/proposal.tsspec/api.mdspeckit/features/008-custom-proposal-producer/contracts/sdk-api.mdspeckit/features/008-custom-proposal-producer/data-model.mdspeckit/features/008-custom-proposal-producer/spec.md
💤 Files with no reviewable changes (1)
- packages/miden-multisig-client/src/multisig/proposal/parser.ts
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (49.72%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #268 +/- ##
==========================================
- Coverage 76.40% 76.04% -0.36%
==========================================
Files 136 136
Lines 24910 25265 +355
==========================================
+ Hits 19032 19214 +182
- Misses 5878 6051 +173 Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Comment on lines
+1032
to
+1035
| if (!/^[a-z0-9_]+$/.test(label)) { | ||
| throw new Error( | ||
| `proposalType '${label}' must be lowercase snake_case ([a-z0-9_]): no spaces, hyphens, or other characters`, | ||
| ); |
Comment on lines
+251
to
+254
| return Err(MultisigError::InvalidConfig(format!( | ||
| "proposal_type '{}' must be lowercase snake_case ([a-z0-9_]): no spaces, hyphens, or other characters", | ||
| proposal_type | ||
| ))); |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
speckit/features/008-custom-proposal-producer/spec.md (1)
102-102: ⚡ Quick winClarify the FR cross-reference for canonical serialization.
FR-020 references “canonical serialization from FR-018,” but FR-018 currently defines return behavior, not serialization guidance. Please either point to the correct FR or add the canonical-serialization definition where referenced.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@speckit/features/008-custom-proposal-producer/spec.md` at line 102, FR-020's cross-reference is incorrect: it points to FR-018 which does not define canonical serialization. Update the spec by either (A) changing the reference in FR-020 from "FR-018" to the correct FR that contains the canonical-serialization definition (replace "FR-018" with the proper FR identifier), or (B) add a short canonical-serialization paragraph into FR-018 (or an adjacent FR) that defines the recommended canonical serialization used for persistence/replay; ensure the text mentions "canonical serialization" and is referenced verbatim by FR-020 so the cross-reference is accurate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@speckit/features/008-custom-proposal-producer/spec.md`:
- Line 97: The normative requirement FR-015 in
speckit/features/008-custom-proposal-producer/spec.md contains a duplicated word
("serialized serialized"); edit the FR-015 sentence to remove the extra
"serialized" so it reads "The serialized transaction request MUST NOT be stored
anywhere..." and keep the rest of the clause intact (references to create and
execute-prep behavior and FR-010 should be unchanged).
---
Nitpick comments:
In `@speckit/features/008-custom-proposal-producer/spec.md`:
- Line 102: FR-020's cross-reference is incorrect: it points to FR-018 which
does not define canonical serialization. Update the spec by either (A) changing
the reference in FR-020 from "FR-018" to the correct FR that contains the
canonical-serialization definition (replace "FR-018" with the proper FR
identifier), or (B) add a short canonical-serialization paragraph into FR-018
(or an adjacent FR) that defines the recommended canonical serialization used
for persistence/replay; ensure the text mentions "canonical serialization" and
is referenced verbatim by FR-020 so the cross-reference is accurate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4d87729e-145a-454a-910c-31c146d25b6c
📒 Files selected for processing (26)
crates/miden-multisig-client/README.mdcrates/miden-multisig-client/src/account.rscrates/miden-multisig-client/src/client/helpers.rscrates/miden-multisig-client/src/client/proposals.rscrates/miden-multisig-client/src/error.rscrates/miden-multisig-client/src/proposal.rscrates/miden-multisig-client/src/transaction/mod.rscrates/server/src/services/mod.rsdocs/MULTISIG_SDK.mdexamples/_shared/multisig-browser/src/multisigApi.tsexamples/demo/src/actions/proposal_management.rsexamples/demo/src/state.rsexamples/smoke-web/README.mdexamples/smoke-web/src/App.tsxexamples/smoke-web/src/smokeHarness.tspackages/guardian-client/src/conversion.test.tspackages/miden-multisig-client/README.mdpackages/miden-multisig-client/src/multisig.tspackages/miden-multisig-client/src/multisig/proposal/execution.tspackages/miden-multisig-client/src/proposal/metadata.test.tspackages/miden-multisig-client/src/proposal/metadata.tspackages/miden-multisig-client/src/types/proposal.tsspec/api.mdspeckit/features/008-custom-proposal-producer/contracts/sdk-api.mdspeckit/features/008-custom-proposal-producer/data-model.mdspeckit/features/008-custom-proposal-producer/spec.md
💤 Files with no reviewable changes (1)
- packages/miden-multisig-client/src/multisig/proposal/execution.ts
✅ Files skipped from review due to trivial changes (6)
- examples/smoke-web/README.md
- crates/miden-multisig-client/README.md
- packages/miden-multisig-client/README.md
- docs/MULTISIG_SDK.md
- speckit/features/008-custom-proposal-producer/contracts/sdk-api.md
- speckit/features/008-custom-proposal-producer/data-model.md
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/guardian-client/src/conversion.test.ts
- crates/miden-multisig-client/src/account.rs
- crates/miden-multisig-client/src/transaction/mod.rs
- examples/demo/src/state.rs
- packages/miden-multisig-client/src/proposal/metadata.ts
- packages/miden-multisig-client/src/proposal/metadata.test.ts
- examples/demo/src/actions/proposal_management.rs
- packages/miden-multisig-client/src/types/proposal.ts
- crates/miden-multisig-client/src/client/helpers.rs
- examples/smoke-web/src/smokeHarness.ts
- examples/smoke-web/src/App.tsx
- crates/miden-multisig-client/src/proposal.rs
| - **FR-012**: Documentation MUST explain the producer flow, the producer's responsibility to reproduce the exact transaction at execution, and that the SDK cannot interpret a custom transaction — so cosigners must review the raw transaction summary, not the label or description. | ||
| - **FR-013**: The end-to-end producer flow (create → sign → execute) for a custom label MUST be demonstrable through at least one example harness. | ||
| - **FR-014**: For v1, the SDK MUST require a full producer-supplied transaction (in serializable form) as the creation input and MUST derive the canonical transaction summary itself; it MUST NOT accept a pre-built transaction summary as a creation input. This keeps the SDK's executability check at creation and the binding re-verification at execution intact. (Accepting a pre-built summary is explicitly out of scope for v1.) | ||
| - **FR-015**: The serialized serialized transaction request MUST NOT be stored anywhere by the SDK or server — not embedded in proposal metadata, not persisted server-side. The integration owns its transaction recipe and supplies the transaction request bytes at create (to derive the summary) and at execute-prep (for the binding check). This keeps the server a minimal coordinator (FR-010) and means custom execution is performed by a party holding the recipe. |
There was a problem hiding this comment.
Fix duplicated wording in normative requirement.
Line 97 says “serialized serialized transaction request,” which reads as an editorial error in a MUST requirement. Please remove the duplicate word.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@speckit/features/008-custom-proposal-producer/spec.md` at line 97, The
normative requirement FR-015 in
speckit/features/008-custom-proposal-producer/spec.md contains a duplicated word
("serialized serialized"); edit the FR-015 sentence to remove the extra
"serialized" so it reads "The serialized transaction request MUST NOT be stored
anywhere..." and keep the rest of the clause intact (references to create and
execute-prep behavior and FR-010 should be unchanged).
Collaborator
|
Can we get this merged soon and get a release? We need it to test the wallet flow for v015 which we want to release next week. Is it possible to get a review from another real person? Atm it is just AI reviewing, right? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Tests