feat(wasm-sdk): expose tiered token pricing in setPrice#3967
feat(wasm-sdk): expose tiered token pricing in setPrice#3967thepastaclaw wants to merge 5 commits into
Conversation
Add backwards-compatible `priceTiers?: Record<string, bigint>` input to TokenSetPriceOptions so callers can configure a TokenPricingSchedule::SetPrices schedule (per-amount tier pricing / minimum-bulk-buy), matching what lower SDKs already support. The flat `price` field is preserved and null still disables direct purchases; specifying both is rejected. Validation rejects empty tier maps, a zero minimum bulk-buy amount, and zero per-token credit prices, each with a useful error pointing callers at the correct field. Includes focused Rust unit tests for the validation helper. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add a focused unit test that asserts TokensFacade.setPrice forwards a `priceTiers` schedule unchanged to wasmSdk.tokenSetPrice, and update the existing flat-price test to the current `price: bigint` shape. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The lower/consensus SetPrices schedule permits zero-credit tiers for free direct purchases at a given bulk amount, but the wasm-sdk wrapper was rejecting them. Drop the zero-credits check while keeping the empty map and zero-amount-key rejections. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughExtends ChangesTiered Direct-Purchase Pricing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
✅ Review complete (commit b9dd057) |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@packages/wasm-sdk/src/state_transitions/token.rs`:
- Around line 2212-2237: The pricing selection in
`deserialize_token_set_price_options` / the pricing schedule builder currently
treats omitted `price` and omitted `priceTiers` the same as an explicit disable,
so require an explicit pricing mode instead. Update the validation around
`price_field_present`, `price_tiers`, and `parsed.price` so that the function
rejects requests where both pricing fields are absent, while still allowing
either `price` or `priceTiers` individually and preserving the existing
mutual-exclusion check. Ensure the error points callers to provide exactly one
of the supported pricing modes.
- Around line 2093-2102: The priceTiers parsing in token.rs currently lets
different string keys like "1" and "01" collapse to the same TokenAmount and be
silently overwritten by tiers.insert. Update the parsing logic around the
key_str.parse flow to detect when a parsed amount already exists in tiers and
return a WasmSdkError::invalid_argument instead of overwriting it. Keep the
check close to the existing priceTiers loop so the error clearly references the
duplicate parsed tier amount and the original key_str.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5025a905-b727-4052-8208-058db19dc440
📒 Files selected for processing (3)
packages/js-evo-sdk/package.jsonpackages/js-evo-sdk/tests/unit/facades/tokens.spec.tspackages/wasm-sdk/src/state_transitions/token.rs
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Small, well-scoped wasm-sdk change adding tiered direct-purchase pricing to tokenSetPrice. Boundary handling (Reflect-based bigint extraction, mutual-exclusion check) is sound, the protocol-level schedule is reused correctly, and there are no consensus/versioning/dependency concerns. Two low-impact suggestions on the new JS boundary: a missing wasm-runtime test for the bigint parser, and a TypeScript-vs-Rust strictness mismatch for tier values.
🟡 2 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/wasm-sdk/src/state_transitions/token.rs`:
- [SUGGESTION] packages/wasm-sdk/src/state_transitions/token.rs:2062-2108: No wasm/JS-runtime test covers the new priceTiers extraction path
`extract_price_tiers` is the only piece of code that exercises the hand-rolled `Reflect::get` + `Object.entries` + `try_to_u64` path for `Record<string, bigint>`. The Rust unit tests only validate an already-constructed `BTreeMap`, and the js-evo-sdk facade spec only asserts stub forwarding. That leaves regressions in bigint→u64 conversion, the both-fields rejection, key parsing, and the SetPrices mapping uncovered by any test that actually crosses the JS/WASM boundary. Add a wasm-bindgen-test (or equivalent web-target test) that calls `tokenSetPrice` with a real JS object containing bigint tier values and asserts both the happy path and the `price`+`priceTiers` rejection.
- [SUGGESTION] packages/wasm-sdk/src/state_transitions/token.rs:2100: priceTiers values are typed as bigint in TS but accepted as Number/String at runtime
The TypeScript declaration at line 1966 says `priceTiers?: Record<string, bigint>`, but `try_to_u64` (wasm-dpp2/src/utils.rs:326) also accepts JS Number and numeric String. For plain JS callers (no TS type-checking), passing a Number > `2^53-1` will already be rounded by JS before Rust sees it, and Rust will then build a `TokenPricingSchedule::SetPrices` with a silently-rounded price. Either gate this path with an explicit `value.is_bigint()` check so the TS contract is enforced at runtime, or relax the TS type to `bigint | number | string` so it matches the implementation. Note this is the same lenient pattern used for the existing flat `price` field, so the team may decide to leave it as-is for consistency.
| fn extract_price_tiers( | ||
| options: &JsValue, | ||
| ) -> Result<Option<BTreeMap<TokenAmount, Credits>>, WasmSdkError> { | ||
| let value = js_sys::Reflect::get(options, &JsValue::from_str("priceTiers")) | ||
| .map_err(|_| WasmSdkError::invalid_argument("failed to read 'priceTiers' from options"))?; | ||
|
|
||
| if value.is_undefined() || value.is_null() { | ||
| return Ok(None); | ||
| } | ||
|
|
||
| if !value.is_object() || js_sys::Array::is_array(&value) { | ||
| return Err(WasmSdkError::invalid_argument( | ||
| "'priceTiers' must be an object mapping token amount keys to bigint credit prices", | ||
| )); | ||
| } | ||
|
|
||
| let obj = js_sys::Object::from(value); | ||
| let entries = js_sys::Object::entries(&obj); | ||
| let mut tiers: BTreeMap<TokenAmount, Credits> = BTreeMap::new(); | ||
|
|
||
| for entry in entries.iter() { | ||
| let entry_arr = js_sys::Array::from(&entry); | ||
| let key = entry_arr.get(0); | ||
| let val = entry_arr.get(1); | ||
|
|
||
| let key_str = key.as_string().ok_or_else(|| { | ||
| WasmSdkError::invalid_argument( | ||
| "'priceTiers' keys must be strings encoding token amounts", | ||
| ) | ||
| })?; | ||
|
|
||
| let amount: TokenAmount = key_str.parse().map_err(|err| { | ||
| WasmSdkError::invalid_argument(format!( | ||
| "Invalid token amount key '{}' in priceTiers: {}", | ||
| key_str, err | ||
| )) | ||
| })?; | ||
|
|
||
| let credits = try_to_u64(&val, &format!("priceTiers['{}']", key_str))?; | ||
|
|
||
| tiers.insert(amount, credits); | ||
| } | ||
|
|
||
| validate_price_tiers(&tiers)?; | ||
|
|
||
| Ok(Some(tiers)) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: No wasm/JS-runtime test covers the new priceTiers extraction path
extract_price_tiers is the only piece of code that exercises the hand-rolled Reflect::get + Object.entries + try_to_u64 path for Record<string, bigint>. The Rust unit tests only validate an already-constructed BTreeMap, and the js-evo-sdk facade spec only asserts stub forwarding. That leaves regressions in bigint→u64 conversion, the both-fields rejection, key parsing, and the SetPrices mapping uncovered by any test that actually crosses the JS/WASM boundary. Add a wasm-bindgen-test (or equivalent web-target test) that calls tokenSetPrice with a real JS object containing bigint tier values and asserts both the happy path and the price+priceTiers rejection.
source: ['codex', 'claude']
There was a problem hiding this comment.
Still present at b9dd057: current head adds pure-Rust helper tests, but still does not exercise the JS↔WASM priceTiers extraction path with a real JS object.
| )) | ||
| })?; | ||
|
|
||
| let credits = try_to_u64(&val, &format!("priceTiers['{}']", key_str))?; |
There was a problem hiding this comment.
🟡 Suggestion: priceTiers values are typed as bigint in TS but accepted as Number/String at runtime
The TypeScript declaration at line 1966 says priceTiers?: Record<string, bigint>, but try_to_u64 (wasm-dpp2/src/utils.rs:326) also accepts JS Number and numeric String. For plain JS callers (no TS type-checking), passing a Number > 2^53-1 will already be rounded by JS before Rust sees it, and Rust will then build a TokenPricingSchedule::SetPrices with a silently-rounded price. Either gate this path with an explicit value.is_bigint() check so the TS contract is enforced at runtime, or relax the TS type to bigint | number | string so it matches the implementation. Note this is the same lenient pattern used for the existing flat price field, so the team may decide to leave it as-is for consistency.
source: ['codex']
There was a problem hiding this comment.
Still present at b9dd057.
…dation - Detect priceTiers keys that parse to the same TokenAmount (e.g. "1" vs "01") instead of silently overwriting via BTreeMap::insert, surfacing both the original and duplicate string keys in the error message. - Require tokenSetPrice callers to specify exactly one of `price` (including explicit `null` for disable) or `priceTiers`, so an empty options object is no longer silently treated like `price: null`. The existing `price` + `priceTiers` rejection is preserved. - Add focused Rust unit tests for both validation paths. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta (c348df2 → 8cb06a9) only adjusts the js-evo-sdk Mocha loader invocation in package.json; token.rs is byte-identical. Both prior priceTiers boundary suggestions still apply at head 8cb06a9 and are carried forward. Codex's new claim that omitting both price fields is a blocking bug is demoted: the same Option<u64> + #[serde(default)] shape existed pre-PR, and the code's own line 2232 comment documents the behavior as intentional, so we surface it as a suggestion to either reject or document omit-both rather than block.
🟡 3 suggestion(s)
Carried-forward prior findings
- [SUGGESTION]
packages/wasm-sdk/src/state_transitions/token.rs:2062-2108: No wasm/JS-runtime test exercises the new priceTiers JS↔WASM path —extract_price_tiersis the only code that drives the hand-rolledReflect::get+Object.entries+try_to_u64extraction forRecord<string, bigint>. Existing Rust unit tests only validate an already-constructedBTreeMap, and the js-evo-sdk facade spec only asserts forwarding to a stub — neither crosses the wasm-bindgen boundary. That leaves bigint→u64 conversion, theprice+priceTiersmutual-exclusion check, key parsing, and the mapping toTokenPricingSchedule::SetPricesuncovered against the only place where attacker/user-controlled JS shapes (arrays, BigInts > u64::MAX, numeric strings) get coerced into a signed state transition. Add a wasm-bindgen-test (or web-target functional test alongsidetests/functional/transitions/tokens.spec.ts) that callstokenSetPricewith a real JS object containing bigint tier values and asserts both the happy path and the both-fields rejection. - [SUGGESTION]
packages/wasm-sdk/src/state_transitions/token.rs:2100: priceTiers values typed as bigint in TS but accepted as Number/String at runtime — The TypeScript declaration at token.rs:1966 advertisespriceTiers?: Record<string, bigint>, buttry_to_u64(wasm-dpp2/src/utils.rs:326) also accepts JSNumberand numericString. A plain-JS caller without TS type-checking who passes aNumbergreater thanNumber.MAX_SAFE_INTEGER(2^53-1) will have V8 silently round the value before Rust sees it; the rounded credit price is then encoded into aTokenPricingSchedule::SetPricesthe caller never intended to sign. Token credit prices routinely exceed 2^53. Either gate this call site with an explicitval.is_bigint()check so the TS contract is enforced at the boundary, or widen the TS type tobigint | number | stringto match the lenient converter. The flatpricefield uses the same lenient pattern, so this is a conscious-consistency call rather than a hard bug — flagging so the team picks deliberately.
New findings in latest delta
- None. The latest delta only changes
packages/js-evo-sdk/package.jsontest-loader flags.
Additional cumulative finding
- [SUGGESTION]
packages/wasm-sdk/src/state_transitions/token.rs:2215-2237: Omitting both price and priceTiers silently disables direct purchases — The PR loosened the TypeScript contract fromprice: bigint | nulltoprice?: bigint | null, so a TS caller can now legally omit bothpriceandpriceTiers. Becauseparsed.priceisOption<u64>with#[serde(default)], that omit-both case falls through topricing_schedule = None, which the lower builder treats as 'no schedule' — equivalent to settingprice: nullto disable direct purchases. The TS doc onpricesays the way to disable is by passingnull, so 'omit both' producing the same destructive effect is undocumented and surprising. The behavior is intentional per the line 2232 comment, but the asymmetry between docs and runtime invites accidental schedule wipes from a typoed field name. Either reject omit-both withWasmSdkError::invalid_argument("Specify either 'price' (flat or null) or 'priceTiers' (tiered)"), or update the TS doc to call out that omitting both is also a disable.
Prior finding reconciliation
prior-1: STILL VALID — token.rs is byte-identical between c348df2 and 8cb06a9 (delta is only js-evo-sdk/package.json). extract_price_tiers (lines 2062–2108) is unchanged and no wasm-bindgen-test or web-target test exercising the JS→WASM priceTiers path was added.prior-2: STILL VALID — Line 2100 still callstry_to_u64(&val, ...)with nois_bigint()precheck.try_to_u64in wasm-dpp2/src/utils.rs:326 still accepts BigInt, Number, and numeric String, so the TS-declaredRecord<string, bigint>is not enforced at the boundary.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
- [SUGGESTION] packages/wasm-sdk/src/state_transitions/token.rs:2062-2108: No wasm/JS-runtime test exercises the new priceTiers JS↔WASM path
`extract_price_tiers` is the only code that drives the hand-rolled `Reflect::get` + `Object.entries` + `try_to_u64` extraction for `Record<string, bigint>`. Existing Rust unit tests only validate an already-constructed `BTreeMap`, and the js-evo-sdk facade spec only asserts forwarding to a stub — neither crosses the wasm-bindgen boundary. That leaves bigint→u64 conversion, the `price`+`priceTiers` mutual-exclusion check, key parsing, and the mapping to `TokenPricingSchedule::SetPrices` uncovered against the only place where attacker/user-controlled JS shapes (arrays, BigInts > u64::MAX, numeric strings) get coerced into a signed state transition. Add a wasm-bindgen-test (or web-target functional test alongside `tests/functional/transitions/tokens.spec.ts`) that calls `tokenSetPrice` with a real JS object containing bigint tier values and asserts both the happy path and the both-fields rejection.
- [SUGGESTION] packages/wasm-sdk/src/state_transitions/token.rs:2100: priceTiers values typed as bigint in TS but accepted as Number/String at runtime
The TypeScript declaration at token.rs:1966 advertises `priceTiers?: Record<string, bigint>`, but `try_to_u64` (wasm-dpp2/src/utils.rs:326) also accepts JS `Number` and numeric `String`. A plain-JS caller without TS type-checking who passes a `Number` greater than `Number.MAX_SAFE_INTEGER` (2^53-1) will have V8 silently round the value before Rust sees it; the rounded credit price is then encoded into a `TokenPricingSchedule::SetPrices` the caller never intended to sign. Token credit prices routinely exceed 2^53. Either gate this call site with an explicit `val.is_bigint()` check so the TS contract is enforced at the boundary, or widen the TS type to `bigint | number | string` to match the lenient converter. The flat `price` field uses the same lenient pattern, so this is a conscious-consistency call rather than a hard bug — flagging so the team picks deliberately.
- [SUGGESTION] packages/wasm-sdk/src/state_transitions/token.rs:2215-2237: Omitting both price and priceTiers silently disables direct purchases
The PR loosened the TypeScript contract from `price: bigint | null` to `price?: bigint | null`, so a TS caller can now legally omit both `price` and `priceTiers`. Because `parsed.price` is `Option<u64>` with `#[serde(default)]`, that omit-both case falls through to `pricing_schedule = None`, which the lower builder treats as 'no schedule' — equivalent to setting `price: null` to disable direct purchases. The TS doc on `price` says the way to disable is by passing `null`, so 'omit both' producing the same destructive effect is undocumented and surprising. The behavior is intentional per the line 2232 comment, but the asymmetry between docs and runtime invites accidental schedule wipes from a typoed field name. Either reject omit-both with `WasmSdkError::invalid_argument("Specify either 'price' (flat or null) or 'priceTiers' (tiered)")`, or update the TS doc to call out that omitting both is also a disable.
Note: GitHub does not allow me to submit APPROVE on my own PR, so this exact-SHA review is posted as COMMENT while preserving the verified suggestions.
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review at b9dd057. The latest delta resolves prior-3 (silent-disable footgun) by introducing validate_pricing_mode_selection and also addresses CodeRabbit's duplicate-key concern via build_price_tiers. Two prior suggestion-level findings remain valid and are carried forward: missing wasm-bindgen/JS-runtime coverage for the priceTiers extraction boundary, and the TS-vs-runtime contract mismatch where priceTiers is typed bigint but try_to_u64 silently accepts JS Number/String. No new blocking issues introduced.
🟡 2 suggestion(s)
Carried-forward findings already raised (1)
These findings were not re-posted as new inline comments because an existing review thread already covers them.
- [SUGGESTION] (deduped existing open thread)
packages/wasm-sdk/src/state_transitions/token.rs:2143-2152: priceTiers values typed as bigint in TS but accepted as Number/String at runtime — The TypeScript declaration at token.rs:1966 sayspriceTiers?: Record<string, bigint>, buttry_to_u64(wasm-dpp2/src/utils.rs:326) lenient-accepts JSNumberand numericStringin addition toBigInt. For plain JS callers without TS type-checking, passing aNumbergreater than2^53 - 1...
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/wasm-sdk/src/state_transitions/token.rs`:
- [SUGGESTION] packages/wasm-sdk/src/state_transitions/token.rs:2111-2156: No wasm-bindgen/JS-runtime test exercises the priceTiers JS↔WASM boundary
`extract_price_tiers` is the only code that crosses the JS↔WASM boundary for this new feature: it does `Reflect::get("priceTiers")`, walks `Object.entries`, parses string keys to `TokenAmount`, and routes values through `try_to_u64` (which itself accepts BigInt/Number/String). The unit tests added in this delta only validate the pure-Rust helpers (`build_price_tiers`, `validate_price_tiers`, `validate_pricing_mode_selection`) against already-parsed inputs, and the js-evo-sdk facade spec at packages/js-evo-sdk/tests/unit/facades/tokens.spec.ts:476-508 only asserts stub forwarding. Regressions in (a) BigInt→u64 conversion, (b) the both-fields rejection wired via `Reflect::get`, (c) array-as-object rejection, (d) key-string parsing, and (e) the final SetPrices vs SinglePrice mapping would not be caught by any test that actually crosses the boundary. Add a wasm-bindgen-test (or equivalent web-target test against the built artifact) that calls `tokenSetPrice` with a real JS object containing bigint tier values and asserts both the happy path and the price+priceTiers rejection.
- [SUGGESTION] packages/wasm-sdk/src/state_transitions/token.rs:2143-2152: priceTiers values typed as bigint in TS but accepted as Number/String at runtime
The TypeScript declaration at token.rs:1966 says `priceTiers?: Record<string, bigint>`, but `try_to_u64` (wasm-dpp2/src/utils.rs:326) lenient-accepts JS `Number` and numeric `String` in addition to `BigInt`. For plain JS callers without TS type-checking, passing a `Number` greater than `2^53 - 1` will already be rounded by V8 before Rust ever sees it, producing a `TokenPricingSchedule::SetPrices` with a silently truncated per-tier credit value at the precise boundary where precision matters most. Either enforce the TS contract at runtime by gating the value path with `val.is_bigint()` before calling `try_to_u64`, or relax the TS type to `Record<string, bigint | number | string>` so the declared contract matches behavior. Note the flat `price` field uses the same lenient pattern via serde, so the choice between strict-runtime and loose-TS should be deliberate for consistency.
|
@coderabbitai review |
✅ Action performedReview finished.
|
Tiered Token Direct-Purchase Pricing
Issue being fixed or feature implemented
The protocol supports consensus-enforced minimum direct-purchase amounts through
TokenPricingSchedule::SetPrices: the lowest tier key is the minimum purchasabletoken amount. Rust SDK, FFI, and lower-level wasm-dpp2 paths can represent tiered
schedules, but the high-level JavaScript path could only call
tokenSetPricewith a flat
price: bigint | null.That meant JS users of
@dashevo/evo-sdk/@dashevo/wasm-sdkcouldUI-enforce "buy at least 100", but could not set the consensus-enforced tier
schedule through the normal SDK API.
What was done?
TokenSetPriceOptionswithpriceTiers?: Record<string, bigint>.priceTierstoTokenPricingSchedule::SetPriceswhile keepingpricemapped toSinglePriceandnullmapped to disabling directpurchases.
priceandpriceTiers.tokens.setPrice()covers both flat pricesand tiered schedules forwarded unchanged to wasm-sdk.
--node-option loader=ts-node/esmpath with ts-node transpile-only runtimeexecution.
How Has This Been Tested?
Validation performed locally on macOS arm64:
cargo fmt --all -- --checkcargo check -p wasm-sdk --testscargo test -p wasm-sdk --lib state_transitions::token::tests::cargo clippy -p wasm-sdk --lib --testsyarn workspace @dashevo/wasm-sdk lintpackages/wasm-sdk/tests/functional/shielded.spec.tsabout_ebeingunused.
yarn workspace @dashevo/evo-sdk lintyarn workspace @dashevo/wasm-sdk buildwasm-optwas not installed locally, so wasm-pack skipped optimizationafter producing the build.
yarn workspace @dashevo/evo-sdk buildYARN_NODE_LINKER=node-modules yarn install --immutablenode_modulesinstall for localpackage test execution in this worktree.
YARN_NODE_LINKER=node-modules yarn workspace @dashevo/evo-sdk test:unitimported built
@dashevo/wasm-sdkand@dashevo/evo-sdkmonkeypatched
wasmSdk.tokenSetPricecalled:
asserted evo-sdk forwards the
priceTiersobject unchangedcode-reviewgate returnedshipbefore opening the PR.code-reviewgate returnedshipagain after adding theevo-sdk test-script fix.
Breaking Changes
None. Existing
price: bigint | nullcallers continue to work.priceTiersisadditive, and passing both fields is rejected as invalid input.
Checklist
corresponding section if my code contains any
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
priceTiers, alongside existing fixedprice.Bug Fixes
price) and tiered pricing (priceTiers).Tests
setPriceoptions shape and added coverage for tier forwarding and validation rules.