wasm: handle spend transaction planner requests in planner #1801
Conversation
🦋 Changeset detectedLatest commit: b1e87c4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
packages/wasm/crate/src/planner.rs
Outdated
| // Constraint: validate the transaction planner request is constructed with a single spend request. | ||
| if request.spends.len() > 1 { | ||
| let error_message = | ||
| "Invalid transaction: The transaction was constructed improperly.".to_string(); |
There was a problem hiding this comment.
suggestion: think we can be more descriptive of the error, like:
"Invalid transaction: only one Spend action allowed in planner request."
There was a problem hiding this comment.
this constraint is triggered when the consumer attempts to construct a TPR with multiple spend requests. The original intention was to use a single, unified error message for the entire feature, but I'll implement more specific and descriptive error messages.
I modified the error message per your suggestion.
packages/wasm/crate/src/planner.rs
Outdated
| let error_message = | ||
| "Invalid transaction: The transaction was constructed improperly.".to_string(); |
There was a problem hiding this comment.
issue: let's reflect the error more specifically here in the error message
There was a problem hiding this comment.
suggesting "Invalid transaction: The requested spend amount does not match the available balance."?
packages/wasm/crate/src/planner.rs
Outdated
| // } | ||
| // } | ||
| // } | ||
| for tpr::Spend { value, address } in request.spends.clone() { |
There was a problem hiding this comment.
question: Why is it necessary to clone()?
There was a problem hiding this comment.
borrow of moved value; we check if request.spends.len() > 1
There was a problem hiding this comment.
question: do you check this case?
- // Constraint: check if the requested spend amount is equal to the accumulated note balance.
There was a problem hiding this comment.
question: are you asserting the change address in a successful result?
There was a problem hiding this comment.
question: do you have any success cases?
There was a problem hiding this comment.
question: do you check this case?
we already check this constraint (see constraint 3 in the test code). To improve test clarity and reduce potential confusion, I reordered the unit tests to match the sequence of constraints as they appear in the planner.
There was a problem hiding this comment.
question: are you asserting the change address in a successful result?
thanks for the suggestion; I'll add assertions around sanity checking the change address.
|
Amazing testing btw! |
references #1660, and partially references #692.
Implements wasm bindgen unit tests for sanity-checking the critical logic paths and safety checks for the send max feature.
#1698 to follow.