fix(sdk): use string keys instead of object keys in JavaScript Maps#3145
Open
fix(sdk): use string keys instead of object keys in JavaScript Maps#3145
Conversation
JavaScript Map uses reference equality for object keys, making lookups fail when a new object is constructed from the same bytes. Convert all IdentifierWasm keys to base58 strings and PlatformAddressWasm keys to hex strings. ProTxHash-based keys use hex to match ProTxHash conventions. Update unchecked_return_type annotations from Map<Identifier, ...> to Map<string, ...> accordingly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughMap keys exposed at the WASM/JS boundary were converted from Identifier/PlatformAddress objects to string representations (base58 or hex) across wasm-dpp2, wasm-sdk queries, state transitions, and corresponding tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/wasm-sdk/src/state_transitions/addresses.rs (1)
33-56:⚠️ Potential issue | 🟡 MinorStale doc comment: still references
Map<PlatformAddress, PlatformAddressInfo>.Line 35 says "to Map<PlatformAddress, PlatformAddressInfo>" but the function now produces
Map<string, PlatformAddressInfo>with hex string keys.Proposed fix
-/// This helper handles the common pattern of converting IndexMap<PlatformAddress, Option<AddressInfo>> -/// to Map<PlatformAddress, PlatformAddressInfo> for WASM bindings. +/// This helper handles the common pattern of converting IndexMap<PlatformAddress, Option<AddressInfo>> +/// to Map<string, PlatformAddressInfo> for WASM bindings, using hex string keys.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/wasm-sdk/src/state_transitions/addresses.rs` around lines 33 - 56, Update the stale doc comment on function address_infos_to_js_map to state that it converts IndexMap<PlatformAddress, Option<AddressInfo>> into a JavaScript Map<string, PlatformAddressInfo> where keys are hex strings (produced by PlatformAddressWasm::from(...).to_hex()) and values are PlatformAddressInfo (PlatformAddressInfoWasm), instead of claiming Map<PlatformAddress, PlatformAddressInfo>; be concise and mention that keys are hex string representations of PlatformAddress.packages/wasm-dpp2/src/state_transitions/proof_result.rs (1)
249-249:⚠️ Potential issue | 🟡 MinorStale type comments not updated to reflect string keys
Both field comments still reference
IdentifierWasmkeys, but after lines 789 and 811 the runtime keys arestring(base58). The comment at line 509 (Map<string(hex), ...>) shows the correct pattern.📝 Proposed fix
pub struct VerifiedTokenIdentitiesBalancesWasm { - balances: Map, // Map<IdentifierWasm, BigInt> + balances: Map, // Map<string, BigInt> }pub struct VerifiedDocumentsWasm { - documents: Map, // Map<IdentifierWasm, DocumentWasm | undefined> + documents: Map, // Map<string, DocumentWasm | undefined> }Also applies to: 322-322
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/wasm-dpp2/src/state_transitions/proof_result.rs` at line 249, Update the stale field comments in proof_result.rs to reflect that runtime map keys are strings (base58) rather than IdentifierWasm: change the comment for the balances field (Map, previously annotated as Map<IdentifierWasm, BigInt>) to indicate Map<string(base58), BigInt> and make the same update for the other affected field referenced around the 322 comment; locate the fields by name (e.g., balances) in the ProofResult struct and replace IdentifierWasm in comments with the correct string(base58) key notation to match the runtime behavior shown elsewhere (e.g., the Map<string(hex), ...> comment).
🧹 Nitpick comments (3)
packages/wasm-sdk/tests/functional/identities.spec.ts (1)
172-179: Add an assertion on the Map key to actually exercise the PR's fix.The updated comment correctly documents that keys are now
string(base58), but the test body never touches the keys — it only readsbalances.values(). A regression where keys revert toIdentifierobjects would silently pass this test.Since
TEST_IDENTITYis already a base58 string, the cleanest fix is to look up viabalances.get(TEST_IDENTITY)directly:✅ Proposed assertion improvement
- // getIdentitiesBalances returns Map<string, bigint | null> where keys are base58-encoded identifiers const balances = await client.getIdentitiesBalances([TEST_IDENTITY]); expect(balances).to.be.instanceOf(Map); expect(balances.size).to.equal(1); + // Keys must be base58 strings — string lookup must succeed (core fix in this PR) + expect(Array.from(balances.keys())[0]).to.be.a('string'); + expect(Array.from(balances.keys())[0]).to.equal(TEST_IDENTITY); + // Retrieve by string key directly + const balance = balances.get(TEST_IDENTITY); - // For existing identity, balance should be present - const balance = Array.from(balances.values())[0]; expect(typeof balance).to.equal('bigint');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/wasm-sdk/tests/functional/identities.spec.ts` around lines 172 - 179, The test currently only inspects values and misses verifying that Map keys are base58 strings; update the assertions around balances (returned by client.getIdentitiesBalances) to directly assert the Map contains the key TEST_IDENTITY (e.g., use balances.get(TEST_IDENTITY) or balances.has(TEST_IDENTITY)) and then assert the retrieved value is a bigint; ensure you still check balances is a Map and its size is 1 but replace the Array.from(balances.values())[0] lookup with a direct lookup using TEST_IDENTITY so the test fails if keys are not the expected base58 string.packages/wasm-sdk/src/queries/token.rs (1)
408-408: Minor: duplicated price schedule →(base_price, current_price)extraction logic.The
match schedule { SinglePrice(..) => ..., SetPrices(..) => ... }block appears identically inget_token_price_by_contract(Lines 204–222),get_token_direct_purchase_prices(Lines 429–446), andget_token_direct_purchase_prices_with_proof_info(Lines 767–784). Consider extracting a small helper to avoid this triplication.Also applies to: 448-452
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/wasm-sdk/src/queries/token.rs` at line 408, Duplicate logic extracting (base_price, current_price) from a PriceSchedule enum is repeated in get_token_price_by_contract, get_token_direct_purchase_prices, and get_token_direct_purchase_prices_with_proof_info; factor it into a small helper like fn extract_prices(schedule: &PriceSchedule) -> (PriceType, PriceType) (use the actual price numeric type in the crate) that matches on SinglePrice and SetPrices and returns (base_price, current_price), then replace the repeated match blocks in those functions with calls to extract_prices and update any imports/visibility accordingly.packages/wasm-dpp2/tests/unit/ProofResult.spec.ts (1)
370-395: Consider asserting value retrieval by string keyThe test currently only checks
size === 1after the round-trip. Adding agetassertion would directly validate the core fix — that lookups by base58 string actually return the expected value from the map produced by the WASM binding.✅ Suggested addition
const result = wasm.VerifiedTokenIdentitiesBalances.fromObject(data); expect(result.balances).to.be.instanceOf(Map); expect(result.balances.size).to.equal(1); + expect(result.balances.get(id1.toBase58())).to.equal(999000n);const result = wasm.VerifiedDocuments.fromObject(data); expect(result.documents).to.be.instanceOf(Map); expect(result.documents.size).to.equal(1); + expect(result.documents.has(id1.toBase58())).to.equal(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/wasm-dpp2/tests/unit/ProofResult.spec.ts` around lines 370 - 395, The test should assert that map lookups by the base58 key return the expected value: after creating id1 (Identifier) and setting balancesMap with id1.toBase58() -> 999000n, add assertions that result.balances.get(id1.toBase58()) === 999000n and that after serializing with result.toObject() and rehydrating via wasm.VerifiedTokenIdentitiesBalances.fromObject(obj) the roundtrip.balances.get(id1.toBase58()) === 999000n so you verify fromObject/toObject preserves key lookup behavior for VerifiedTokenIdentitiesBalances.balances.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/wasm-sdk/src/queries/epoch.rs`:
- Around line 521-525: The map that builds ProTxHash keys in the proof-info
variants uses IdentifierWasm::from(identifier).to_base58(), causing inconsistent
key formats versus the non-proof variants; update those occurrences (the closure
building the Map from counts.0.into_iter() in the proof-info methods such as
getEvonodesProposedEpochBlocksByIdsWithProofInfo and the other proof-info
helper) to use .to_hex() instead of .to_base58() so ProTxHash keys match the
ProTxHashWasm hex convention used elsewhere.
In `@packages/wasm-sdk/tests/functional/epochs-blocks.spec.ts`:
- Around line 103-107: The comment is incorrect: byRange keys are hex strings
(created via IdentifierWasm::from(identifier).to_hex()), not base58. Update the
comment near byRange.keys() to state the keys are hex, and when you pass that
key (testProTxHash) into getEvonodesProposedEpochBlocksByIdsWithProofInfo (which
expects base58 keys) convert the hex key to base58 first (e.g., decode the hex
Identifier and call its to_base58 / toBase58 equivalent) so the key formats
match between getEvonodesProposedEpochBlocksByRange and
getEvonodesProposedEpochBlocksByIdsWithProofInfo.
---
Outside diff comments:
In `@packages/wasm-dpp2/src/state_transitions/proof_result.rs`:
- Line 249: Update the stale field comments in proof_result.rs to reflect that
runtime map keys are strings (base58) rather than IdentifierWasm: change the
comment for the balances field (Map, previously annotated as Map<IdentifierWasm,
BigInt>) to indicate Map<string(base58), BigInt> and make the same update for
the other affected field referenced around the 322 comment; locate the fields by
name (e.g., balances) in the ProofResult struct and replace IdentifierWasm in
comments with the correct string(base58) key notation to match the runtime
behavior shown elsewhere (e.g., the Map<string(hex), ...> comment).
In `@packages/wasm-sdk/src/state_transitions/addresses.rs`:
- Around line 33-56: Update the stale doc comment on function
address_infos_to_js_map to state that it converts IndexMap<PlatformAddress,
Option<AddressInfo>> into a JavaScript Map<string, PlatformAddressInfo> where
keys are hex strings (produced by PlatformAddressWasm::from(...).to_hex()) and
values are PlatformAddressInfo (PlatformAddressInfoWasm), instead of claiming
Map<PlatformAddress, PlatformAddressInfo>; be concise and mention that keys are
hex string representations of PlatformAddress.
---
Nitpick comments:
In `@packages/wasm-dpp2/tests/unit/ProofResult.spec.ts`:
- Around line 370-395: The test should assert that map lookups by the base58 key
return the expected value: after creating id1 (Identifier) and setting
balancesMap with id1.toBase58() -> 999000n, add assertions that
result.balances.get(id1.toBase58()) === 999000n and that after serializing with
result.toObject() and rehydrating via
wasm.VerifiedTokenIdentitiesBalances.fromObject(obj) the
roundtrip.balances.get(id1.toBase58()) === 999000n so you verify
fromObject/toObject preserves key lookup behavior for
VerifiedTokenIdentitiesBalances.balances.
In `@packages/wasm-sdk/src/queries/token.rs`:
- Line 408: Duplicate logic extracting (base_price, current_price) from a
PriceSchedule enum is repeated in get_token_price_by_contract,
get_token_direct_purchase_prices, and
get_token_direct_purchase_prices_with_proof_info; factor it into a small helper
like fn extract_prices(schedule: &PriceSchedule) -> (PriceType, PriceType) (use
the actual price numeric type in the crate) that matches on SinglePrice and
SetPrices and returns (base_price, current_price), then replace the repeated
match blocks in those functions with calls to extract_prices and update any
imports/visibility accordingly.
In `@packages/wasm-sdk/tests/functional/identities.spec.ts`:
- Around line 172-179: The test currently only inspects values and misses
verifying that Map keys are base58 strings; update the assertions around
balances (returned by client.getIdentitiesBalances) to directly assert the Map
contains the key TEST_IDENTITY (e.g., use balances.get(TEST_IDENTITY) or
balances.has(TEST_IDENTITY)) and then assert the retrieved value is a bigint;
ensure you still check balances is a Map and its size is 1 but replace the
Array.from(balances.values())[0] lookup with a direct lookup using TEST_IDENTITY
so the test fails if keys are not the expected base58 string.
- Use ProTxHashWasm for ProTxHash map keys in epoch.rs instead of IdentifierWasm, with hex encoding matching ProTxHash conventions - Add From<Identifier> for ProTxHashWasm conversion - Fix stale doc comments in addresses.rs, proof_result.rs, and epochs-blocks.spec.ts to reflect string key types Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/wasm-sdk/src/state_transitions/addresses.rs (1)
219-222:addressInfosgetters on result structs are missingunchecked_return_typeafter the key-type change.The PR updates
unchecked_return_typefor the three methods that return aMapdirectly (addressFundsTransfer,addressFundsWithdraw,addressFundingFromAssetLock), but theaddressInfosgetters onIdentityTopUpFromAddressesResultWasm(line 219),IdentityTransferToAddressesResultWasm(line 564), andIdentityCreateFromAddressesResultWasm(line 808) all call the sameaddress_infos_to_js_maphelper and now serveMap<string, PlatformAddressInfo>without advertising that to TypeScript consumers.♻️ Proposed additions for all three getters
- #[wasm_bindgen(getter = "addressInfos")] + #[wasm_bindgen(getter = "addressInfos", unchecked_return_type = "Map<string, PlatformAddressInfo>")] pub fn address_infos(&self) -> Map {Apply identically at lines 219, 564, and 808.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/wasm-sdk/src/state_transitions/addresses.rs` around lines 219 - 222, The addressInfos getters on IdentityTopUpFromAddressesResultWasm, IdentityTransferToAddressesResultWasm, and IdentityCreateFromAddressesResultWasm call address_infos_to_js_map and now return a Map<string, PlatformAddressInfo> but lack the unchecked_return_type annotation; update each getter's attribute to #[wasm_bindgen(getter = "addressInfos", unchecked_return_type)] so the TypeScript signature reflects the actual keyed value type (apply the change to the address_infos method on each of those three structs).packages/wasm-dpp2/src/core/pro_tx_hash.rs (1)
160-164: Consider adding a doc comment clarifying the semantic constraint.This
From<Identifier>impl silently reinterprets any 32-byteIdentifieras aProTxHash. It is semantically valid only when theIdentifierwraps actual ProTxHash bytes (e.g., evonode IDs fromProposerBlockCountById). Without a comment, future callers may invoke this conversion for non-ProTxHash identifiers and get a well-typed but semantically wrong value.🔧 Suggested clarifying doc comment
+/// Reinterprets the raw 32-byte buffer of an `Identifier` as a `ProTxHash`. +/// +/// # Safety / Correctness +/// This conversion is only semantically meaningful when the `Identifier` actually +/// represents an evonode ProTxHash (e.g., keys from `ProposerBlockCountById`). +/// Using it for arbitrary identifiers (document IDs, contract IDs, etc.) will +/// produce a structurally valid but meaningless `ProTxHashWasm`. impl From<dpp::platform_value::Identifier> for ProTxHashWasm { fn from(id: dpp::platform_value::Identifier) -> Self { ProTxHashWasm(ProTxHash::from_byte_array(id.to_buffer())) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/wasm-dpp2/src/core/pro_tx_hash.rs` around lines 160 - 164, Add a doc comment on the impl From<dpp::platform_value::Identifier> for ProTxHashWasm explaining that the conversion simply reinterprets the 32-byte Identifier bytes via ProTxHash::from_byte_array and is only semantically valid when the Identifier actually contains ProTxHash bytes (e.g., evonode IDs from ProposerBlockCountById); indicate that using this conversion on other Identifier values produces a well-typed but semantically incorrect ProTxHash and callers should validate the identifier source before converting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/wasm-dpp2/src/core/pro_tx_hash.rs`:
- Around line 160-164: Add a doc comment on the impl
From<dpp::platform_value::Identifier> for ProTxHashWasm explaining that the
conversion simply reinterprets the 32-byte Identifier bytes via
ProTxHash::from_byte_array and is only semantically valid when the Identifier
actually contains ProTxHash bytes (e.g., evonode IDs from
ProposerBlockCountById); indicate that using this conversion on other Identifier
values produces a well-typed but semantically incorrect ProTxHash and callers
should validate the identifier source before converting.
In `@packages/wasm-sdk/src/state_transitions/addresses.rs`:
- Around line 219-222: The addressInfos getters on
IdentityTopUpFromAddressesResultWasm, IdentityTransferToAddressesResultWasm, and
IdentityCreateFromAddressesResultWasm call address_infos_to_js_map and now
return a Map<string, PlatformAddressInfo> but lack the unchecked_return_type
annotation; update each getter's attribute to #[wasm_bindgen(getter =
"addressInfos", unchecked_return_type)] so the TypeScript signature reflects the
actual keyed value type (apply the change to the address_infos method on each of
those three structs).
shumkov
changed the title
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
1 participant
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.
Issue being fixed or feature implemented
JavaScript
Mapuses reference equality for object keys. When Rust WASM bindings create Maps withIdentifierWasmorPlatformAddressWasmobjectsas keys, the resulting JS Maps are unusable —
map.get(new Identifier(sameBytes))returnsundefinedbecause it's a different object instance.What was done?
Converted all WASM Map keys from object instances to string primitives across
wasm-sdkandwasm-dpp2:Identifierkeys →.to_base58()string (35 locations across 10 Rust files)PlatformAddresskeys →.to_hex()string (address.rs,addresses.rs)epoch.rs→.to_hex()to matchProTxHashWasmhex conventionunchecked_return_typeannotations fromMap<Identifier, ...>/Map<PlatformAddress, ...>toMap<string, ...>Identifierobject creationRust files:
token.rs(10 fixes),group.rs(7),identity.rs(4),epoch.rs(4),address.rs(2),document.rs(2),data_contract.rs(2),
votes.rs(1),addresses.rs(1),proof_result.rs(2)How Has This Been Tested?
cargo check -p wasm-sdkandcargo check -p wasm-dpp2— compile cleanwasm-dpp2:Group.spec.ts,ProofResult.spec.ts,TokenConfiguration.spec.ts,TokenDistributionRules.spec.ts,TokenPreProgrammedDistribution.spec.ts,js-value-to-json.spec.tswasm-sdk:epochs-blocks.spec.ts,identities.spec.tsBreaking Changes
Map keys returned by all
wasm-sdkquery methods andwasm-dpp2proof result methods change fromIdentifier/PlatformAddressobjects tostring. Consumers that previously called.toBase58()or.toHex()on map keys should use the key directly. Consumers that compared keys byreference were already broken.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
API Changes
Tests