feat: enable Platform address state transitions - #71
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
💤 Files with no reviewable changes (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughPlatform address transitions are enabled across schemas, authentication metadata, SDK operation handlers, catalogs, reference examples, and tests. The implementation adds validation, signer construction, address parsing, nonce retry, result serialization, and end-to-end lifecycle coverage. ChangesPlatform address transitions
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant TransitionUI
participant addressTransitionOperations
participant AddressState
participant SDKAddresses
TransitionUI->>addressTransitionOperations: prepare transition inputs
addressTransitionOperations->>AddressState: fetch address or identity data
addressTransitionOperations->>SDKAddresses: execute address operation
SDKAddresses-->>addressTransitionOperations: return transition result
addressTransitionOperations-->>TransitionUI: serialized result and address information
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
Remove the 'disabled' flags gating Platform address operations and update their inputs to the SDK's WIF-based key model, dropping the manual senderNonce fields. Expose the additional SDK exports (CoreScript, PlatformAddress, PoolingWasm, ensureInitialized) needed by the address handler, add asset-lock proof auth wiring for addressFundFromAssetLock, and cover the address operations with unit and e2e tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
f91668b to
6aecf2a
Compare
|
ℹ️ Review superseded (commit 8645818) |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
public/src/transitions/address-operations.js (2)
92-100: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNonce/revision retry relies on brittle error-message matching.
withNonceRetrydetects retryable failures via/nonce|revision/i.test(String(error?.message || error)). If the SDK changes wording, wraps errors, or localizes messages, the retry silently stops firing and callers get a hard failure instead of a rebuild+retry.Please check whether
@dashevo/evo-sdkexposes a typed/coded error (e.g. an error class orcodeproperty) for nonce/revision conflicts that could be matched instead of the message 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 `@public/src/transitions/address-operations.js` around lines 92 - 100, Update withNonceRetry to detect nonce/revision conflicts using the typed or coded error contract exposed by `@dashevo/evo-sdk`, rather than matching error.message text. Inspect the SDK’s error class/code and use that stable identifier in the retry condition, while preserving the existing prepared.rebuild guard, rebuild, retry, and rethrow behavior for non-matching errors.
102-134: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winHand-rolled Base58Check decoding for Core addresses.
coreScriptreimplements Base58 decoding, leading-zero handling, and double-SHA256 checksum verification from scratch. The version bytes (0x4c/0x8cP2PKH,0x10/0x13P2SH) are correct per Dash's documented address conversion scheme, but this is exactly the kind of address/checksum parsing that's easy to get subtly wrong on edge cases and better delegated to a vetted library or an existing SDK helper.Please check whether
@dashevo/evo-sdk'sCoreScript(or another exported helper) already offers afromAddress/parse method that performs this decoding, to avoid maintaining bespoke Base58Check logic for money-moving address parsing.Separately, note this function accepts both mainnet (
0x4c/0x10) and testnet (0x8c/0x13) prefixes without checking the address against the SDK's configured network, so a testnet address could be accepted while connected to mainnet (or vice versa) without an early, friendly validation error.🤖 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 `@public/src/transitions/address-operations.js` around lines 102 - 134, Replace the bespoke Base58Check decoding in coreScript with the vetted `@dashevo/evo-sdk` CoreScript.fromAddress or equivalent exported address parser, preserving the existing CoreScript result and error behavior. Use the SDK/configured network validation so mainnet and testnet prefixes are rejected when they do not match the active network, and remove the now-unneeded manual decoding and checksum logic.
🤖 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 `@public/api-definitions.json`:
- Around line 2497-2502: Change the input type for both addressPrivateKeyWif and
identityPrivateKeyWif from text to password in the corresponding API definition
entries, matching the existing masked private-key field behavior.
In `@public/src/transitions/address-operations.js`:
- Line 198: Validate the non-empty `values.coreFeePerByte` value before
constructing the return object in the address-operation flow, rejecting
non-numeric or otherwise invalid values with a clear client-side validation
error instead of passing `NaN` to `sdk.addresses.withdraw`; preserve `undefined`
for empty or null values.
- Around line 248-264: Update addressCreateIdentity’s prepare and execute
methods to match the nonce/revision retry flow used by addressTransfer,
addressTopUpIdentity, and addressWithdraw: return a rebuild closure that
recreates the spending input and prepared identity options, then invoke
createIdentity through withNonceRetry so conflicts rebuild the consumed
platform-address input before retrying.
In `@tests/e2e/.env.example`:
- Around line 11-16: Add TEST_PLATFORM_ASSET_LOCK_PROOF and
TEST_PLATFORM_ASSET_LOCK_PRIVATE_KEY to the Platform Address lifecycle block in
tests/e2e/.env.example. The references in
tests/e2e/fixtures/test-data.js:956-971 and
tests/e2e/transitions/state-transitions.spec.js:1321-1327 require these
variables; no direct changes are needed there.
---
Nitpick comments:
In `@public/src/transitions/address-operations.js`:
- Around line 92-100: Update withNonceRetry to detect nonce/revision conflicts
using the typed or coded error contract exposed by `@dashevo/evo-sdk`, rather than
matching error.message text. Inspect the SDK’s error class/code and use that
stable identifier in the retry condition, while preserving the existing
prepared.rebuild guard, rebuild, retry, and rethrow behavior for non-matching
errors.
- Around line 102-134: Replace the bespoke Base58Check decoding in coreScript
with the vetted `@dashevo/evo-sdk` CoreScript.fromAddress or equivalent exported
address parser, preserving the existing CoreScript result and error behavior.
Use the SDK/configured network validation so mainnet and testnet prefixes are
rejected when they do not match the active network, and remove the now-unneeded
manual decoding and checksum logic.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 78addb80-19d2-4351-bb9b-21bdd2b6fad2
📒 Files selected for processing (13)
public/AI_REFERENCE.mdpublic/api-definitions.jsonpublic/sdk-operation-catalog.jsonpublic/src/definitions-data.jspublic/src/sdk-types.jspublic/src/transitions/address-operations.jstests/e2e/.env.exampletests/e2e/fixtures/test-data.jstests/e2e/queries/query-execution.spec.jstests/e2e/transitions/state-transitions.spec.jstests/e2e/utils/sdk-page.jstests/unit/address-operations.test.jstests/unit/transition-operations.test.js
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
public/documentation-check-report.txt (1)
4-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid committing a wall-clock timestamp in the tracked report.
scripts/check_documentation.pyregenerates this line withdatetime.now().isoformat(), so every documentation check produces a new diff even when documentation is unchanged. Remove the timestamp from the committed report or publish this report as a CI artifact instead.🤖 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 `@public/documentation-check-report.txt` at line 4, Remove the generated wall-clock Timestamp line from the tracked documentation-check report, or stop tracking the report and publish it only as a CI artifact. Ensure scripts/check_documentation.py output no longer creates recurring diffs for unchanged documentation.tests/e2e/.env.example (1)
17-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReorder the asset-lock variables.
dotenv-linterrequiresTEST_PLATFORM_ASSET_LOCK_PRIVATE_KEYto appear beforeTEST_PLATFORM_ASSET_LOCK_PROOF.Proposed fix
-TEST_PLATFORM_ASSET_LOCK_PROOF=YOUR_CONSUMABLE_ASSET_LOCK_PROOF TEST_PLATFORM_ASSET_LOCK_PRIVATE_KEY=YOUR_ASSET_LOCK_PRIVATE_KEY_WIF +TEST_PLATFORM_ASSET_LOCK_PROOF=YOUR_CONSUMABLE_ASSET_LOCK_PROOF🤖 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 `@tests/e2e/.env.example` around lines 17 - 18, Reorder the asset-lock environment variables in the example configuration so TEST_PLATFORM_ASSET_LOCK_PRIVATE_KEY appears before TEST_PLATFORM_ASSET_LOCK_PROOF, while preserving both variable names and placeholder values.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@public/documentation-check-report.txt`:
- Line 4: Remove the generated wall-clock Timestamp line from the tracked
documentation-check report, or stop tracking the report and publish it only as a
CI artifact. Ensure scripts/check_documentation.py output no longer creates
recurring diffs for unchanged documentation.
In `@tests/e2e/.env.example`:
- Around line 17-18: Reorder the asset-lock environment variables in the example
configuration so TEST_PLATFORM_ASSET_LOCK_PRIVATE_KEY appears before
TEST_PLATFORM_ASSET_LOCK_PROOF, while preserving both variable names and
placeholder values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c243d1c5-6876-41a7-a961-992a8a10b253
📒 Files selected for processing (5)
public/api-definitions.jsonpublic/documentation-check-report.txtpublic/src/transitions/address-operations.jstests/e2e/.env.exampletests/unit/address-operations.test.js
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/unit/address-operations.test.js
- public/api-definitions.json
- public/src/transitions/address-operations.js
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The Platform address implementation is generally well structured, but three verified runtime and documentation contract mismatches prevent advertised flows from working correctly. Asset-lock funding always builds a structurally invalid transition, an empty optional withdrawal fee fails SDK deserialization, and the generated withdrawal example references missing symbols and an input the UI never supplies.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 blocking
🤖 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 `public/src/transitions/address-operations.js`:
- [BLOCKING] public/src/transitions/address-operations.js:249-250: Build a remainder output and valid fee strategy for asset-lock funding
Asset-lock funding requires exactly one output with no `amount`, which becomes the remainder recipient. This code always supplies an explicit amount, so SDK structure validation rejects the transition with `Exactly one output must have None value (remainder recipient)`. The facade also constructs this transition with no address inputs, while an omitted `feeStrategy` defaults to `DeductFromInput(0)`, which is out of bounds for the empty input set. Every request built by this operation therefore fails before broadcast. Emit the single recipient without an amount, pass a strategy such as `FeeStrategyStep.reduceOutput(0)`, and remove the now-inapplicable required `amount` input from `public/api-definitions.json`, fixtures, and the generated example.
- [BLOCKING] public/src/transitions/address-operations.js:205: Default the optional withdrawal fee before calling the SDK
`coreFeePerByte` is optional in `public/api-definitions.json`, and the displayed placeholder of `1` is not submitted as a value. Leaving the field empty produces `null`, which this helper converts to `undefined`, but the options object still includes that property. The installed SDK deserializes `coreFeePerByte` into a required `u32`, so withdrawal preparation fails whenever the user accepts the form's advertised optional behavior. Apply the displayed default here or mark the field required.
- [BLOCKING] public/src/transitions/address-operations.js:216: Generate an executable withdrawal example
The generated snippet imports only `PlatformAddressSigner` and `PrivateKey`, then references `CoreScript` and `PoolingWasm` without importing them. It also uses `coreAddressHash`, which is neither defined in the example nor supplied by the UI; the documented input is the Base58Check `toAddress`. This broken renderer is used by the UI code preview and propagated into `public/AI_REFERENCE.md`, so copying the example raises a `ReferenceError` before the SDK call. Import the required SDK symbols and generate a `toAddress` conversion equivalent to the runtime `coreScript()` path.
Default an omitted withdrawal core fee per byte to 1 instead of sending undefined to the SDK's required u32 field. Generate an executable withdrawal example: import CoreScript/PoolingWasm and derive the output script from the Base58Check toAddress via a self-contained helper rather than the undefined coreAddressHash. Build asset-lock funding with a single remainder output (no amount) and a reduceOutput(0) fee strategy, and drop the now-inapplicable amount input.
2 participants
Summary
Platform address state transitions were previously stubbed out and marked
disabled, throwing onprepare/execute. This PR implements them against the installed SDK so all six can be run from the UI.Changes
address-operations.js— fullprepare/execute/renderCodeimplementations replacing the disabled stubs. Each derives the sender address from its WIF viaPlatformAddressSigner, fetches on-chain address info, validates balance/amount, and serializes returnedaddressInfos. Includes:withNonceRetry) that rebuilds options and resendsCoreScriptdecoder (P2PKH/P2SH) for the withdraw output scriptapi-definitions.json— removed thedisabledflags and the manualsenderNonceinputs (now derived on-chain); switched key inputs to WIF-based fields (addressPrivateKeyWif,identityPrivateKeyWif) and addedaddressPrivateKeyWifto the asset-lock funding form.definitions-data.js— updated auth requirements to target the new WIF fields; added the asset-lock proof requirement foraddressFundFromAssetLock.sdk-types.js— re-exportCoreScript,PlatformAddress,PoolingWasm, andensureInitialized.Testing
tests/unit/address-operations.test.js(216 lines) covering the operations and validation paths..env.exampledocuments the newTEST_PLATFORM_ADDRESS_*/_KEY_*variables (each address must match its WIF).Summary by CodeRabbit