Conversation
There was a problem hiding this comment.
First comments on the presentation / devxp and release cycles.
Note: LSP has the exact same needs (embed a binary and release extensions per target).
We'll need to move to a CI environment in the future for the release cycle as this isn't manageable manually.
| @@ -9,6 +22,7 @@ | |||
| - Build the extension once with `npm run compile` (or keep it rebuilding with `npm run watch`). | |||
There was a problem hiding this comment.
We should mention: npm run bundle:adapter somewhere in the flow, otherwise compilation fails for this section.
| "watch:tsc": "tsc --noEmit --watch --project tsconfig.json", | ||
| "package": "npm run check-types && npm run lint && node esbuild.js --production", | ||
| "prepare-release": "npm run bundle:adapter && npm run package", | ||
| "package:vsix": "npm exec vsce package -- --pre-release", |
There was a problem hiding this comment.
I'd recommend if we enforce manual usage of this command and its parameters, explanation as a comment in the README.md
| "watch:esbuild": "node esbuild.js --watch", | ||
| "watch:tsc": "tsc --noEmit --watch --project tsconfig.json", | ||
| "package": "npm run check-types && npm run lint && node esbuild.js --production", | ||
| "prepare-release": "npm run bundle:adapter && npm run package", |
There was a problem hiding this comment.
Is there a need to have this behavior as a sub script instead of copy/pasting its content to vscode:prepublish?
|
|
||
| ```bash | ||
| npm run package:vsix | ||
| ``` |
There was a problem hiding this comment.
When embedding binaries into the extension, we must publish the extension with a target platform (win, darwin, linux, etc).
Usage: vsce package --target [target]
win32-x64, win32-arm64, linux-x64, linux-arm64, linux-armhf, darwin-x64, darwin-arm64, alpine-x64, alpine-arm64, web
I suggest we don't provide automatic command for that and only some common cmd list usages. Anyway, only the publisher cares about this command, maybe we should even move it to the end of the document?
|
|
||
| Then in the Extension Development Host, press `Ctrl+R` to reload and apply parser/query updates. | ||
|
|
||
| ### Contract Debugging |
There was a problem hiding this comment.
You're right about providing usage documentation here, I even think this should come first in the README as this is what's being displayed on the store (and from the extension page within the IDE).
| - `SILVERSCRIPT_VSCODE_TARGET`, for example `darwin-arm64` or `linux-x64` | ||
| - `SILVERSCRIPT_CARGO_TARGET`, for example `aarch64-apple-darwin` | ||
|
|
||
| Then run `npm run package:vsix`. |
There was a problem hiding this comment.
This is redundant with the first file line
| function escapeHtml(value) { | ||
| return String(value ?? "") | ||
| .replace(/&/g, "&") | ||
| .replace(/"/g, """) | ||
| .replace(/</g, "<") | ||
| .replace(/>/g, ">"); | ||
| } |
There was a problem hiding this comment.
ideal way is to no sanitize anything and display content as DOMNode.textContent
i see this is used to be put into raw html, in this case, i'd use DOMPurify from "dompurify"
| @@ -0,0 +1,47 @@ | |||
| <!DOCTYPE html> | |||
There was a problem hiding this comment.
Could this webapp be using a lightweight framework to simplify readability / expressiveness?
astro/react/vue/svelte?
| - `keypair<N>.pubkey` | ||
| - `keypair<N>.secret` | ||
| - `keypair<N>.pkh` | ||
|
|
There was a problem hiding this comment.
I'd suggest declaring consistent naming: pk, sk, pkHash or publicKey, secretKey, publicKeyHash
secret alone can have different interpretations, especially if coupled with a wallet. But it doesn't matter that much if we provide abstractions on top (like the panel that edit these values for you)
| logDebugEnabled = | ||
| context.extensionMode !== vscode.ExtensionMode.Production; |
There was a problem hiding this comment.
Could probably come from an optional extension settings
| const semanticEnabled = vscode.workspace | ||
| .getConfiguration("silverscript") | ||
| .get<boolean>("enableSemanticTokens", true); | ||
| logInfo(`enableSemanticTokens=${semanticEnabled}`); |
There was a problem hiding this comment.
I removed it in LSP as we have no use for it, but you can keep it and I'll remove later
| Array.isArray(value) || | ||
| (value !== null && typeof value === "object") |
There was a problem hiding this comment.
Is it true that launch args are expected to be array | object, instead of one or the other explicitly?
| if (Array.isArray(input)) { | ||
| for (const [index, param] of params.entries()) { | ||
| if (index < input.length) { | ||
| defaults[param.name] = stringifyLaunchArg(input[index]); | ||
| } | ||
| } | ||
| return defaults; | ||
| } | ||
| if (input && typeof input === "object") { | ||
| for (const param of params) { | ||
| if (Object.prototype.hasOwnProperty.call(input, param.name)) { | ||
| defaults[param.name] = stringifyLaunchArg(input[param.name]); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Same question here for array | object
| model: ContractModel, | ||
| scriptPath: string, | ||
| ): string { | ||
| return model.name && model.name !== "Unknown" |
There was a problem hiding this comment.
In what cases the contract model name in unknown?
Shouldn't parseContractModel reject the parsing if name is not set?
| function resolvedLaunchName( | ||
| state: PanelHostState, | ||
| ): string { | ||
| return typeof state.baseConfig.name === "string" && |
There was a problem hiding this comment.
name is expected to be a string already
| if ( | ||
| restoringPrimaryEditor || | ||
| !panel || | ||
| !editor || | ||
| editor.document.languageId !== "silverscript" || | ||
| editor.viewColumn === vscode.ViewColumn.One || | ||
| panel.viewColumn === undefined || | ||
| editor.viewColumn !== panel.viewColumn | ||
| ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
I'm not able to understand what this is for, I think it's worth adding a small comment
|
Investigated the issue on windows where upon executing the debugger it would run in a new document: If we truly need canonicalize (basically resolve symlinks), I suggest we add a platform specific handling to remove prefix (on win). If we don't need, we can just return absolute path instead of canonicalize. |
| function normalizePath(fsPath: string): string { | ||
| const normalized = path.normalize(fsPath); | ||
| return process.platform === "win32" | ||
| ? normalized.toLowerCase() | ||
| : normalized; | ||
| } |
There was a problem hiding this comment.
This function only is used in one place, in launchConfigMatchesScript, I think we can afford .toLowerCase() on the calling site regardless of the platform:
record.resolvedScriptPath.toLowerCase() === scriptUri.fsPath.toLowerCase()And, now we realize we can also remove launchConfigMatchesScript (by the way the export key is useless because not used externally) and move the logic directly into: listMatchingSilverScriptLaunchConfigs
| } | ||
|
|
||
| records.push({ | ||
| id: `${folder.uri.toString()}::${index}`, |
There was a problem hiding this comment.
toString(true) (skip encoding) seems more appropriate for an id
| export type RawLaunchConfiguration = vscode.DebugConfiguration & Record<string, unknown>; | ||
|
|
||
| export type SilverScriptLaunchConfigRecord = { | ||
| id: string; |
There was a problem hiding this comment.
We can add a comment to inform the format: fspath::config_index
| import * as path from "path"; | ||
| import * as vscode from "vscode"; | ||
|
|
||
| export type RawLaunchConfiguration = vscode.DebugConfiguration & Record<string, unknown>; |
There was a problem hiding this comment.
Ideally we also have a LaunchConfiguration type, with an associated type-guard function (isLaunchConfiguration), so that calling site can rely on pre validated configuration properties, this will remove some complexity found in different places.
Then we can make RawLaunchConfiguration a local type (not exported), since only the surfaced type will be relevant to external readers.
Small catch: internal (to this file) writer still want the ability to operate on "raw" or "unvalidated" configs, example: I have a non valid configuration file, but I still want to be able to create a new configuration (from the webview panel) - in this context, I don't need validation, I read the number of existing configuration, push the fresh one, and write the whole
| if (record.index >= configs.length) { | ||
| throw new Error(`Launch config '${record.config.name ?? record.id}' no longer exists.`); | ||
| } | ||
|
|
||
| configs[record.index] = nextConfig; |
There was a problem hiding this comment.
Using index as the identification key is a mistake, it's not reliable (can change between a read/write)
(same in deleteSilverScriptLaunchConfig)
| typeof nextConfig.scriptPath === "string" | ||
| ? nextConfig.scriptPath | ||
| : record.scriptPathValue, |
There was a problem hiding this comment.
When not using RawLaunchConfiguration but a pre-validated LaunchConfiguration, all of these call are simplified
| if (expanded.includes("${")) { | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
Seems weird, I investigated a bit deeper, it seems expandPathVariables should not be accepting undefined values at the first place, this makes replacements deterministic (if there are placeholders, then they are filled for sure).
expandPathVariables is called by resolveLaunchScriptPath while also wrongly takes args as optionals (they should always be present; or there should be a validation happening prior this function)
| if (!expanded) { | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
With the above comment, this shouldn't happens (unless there is a possible unfilled placeholder I'm not familiar with: and in this case, it should throw an error anyway)
2 participants
DAP crate & a Visual Studio Code debugger for SilverScript.
VS Code uses uses the Debug Adapter Protocol to talk to debuggers for breakpoints, stepping, locals, stack traces, and launch requests.
This PR adds a new
debugger/dapcrate which builds on top of the debugger introduced in #43 and exposes it through DAP.The adapter can be launched by VS Code, or through CLI with
--run-config-json. That CLI path exists so the same launch shape can be used from tests, scripts or AI agents.Pressing
F5, or using the inline CodeLens above a contract, opens a panel for choosing the entrypoint, filling constructor and function args, and then either running or debugging the contract.The panel integrates with
launch.jsonso scenarios can be loaded and saved instead of re-entering args each time. CodeLens surfaces saved-scenario state directly above each entrypoint with aN scenarios savedsummary, and clicking that count opens the same side panel and jumps straight into picking a saved scenario for that entrypoint when one exists. Advanced fields liketxstay inlaunch.json, while the panel focuses on the common run/debug path.This PR also adds symbolic identities for contracts that take key-related inputs. Launch args can use placeholders like
keypair1.pubkey,keypair1.secret, andkeypair1.pkh, and the adapter resolves them at launch time into a matching keypair. This keeps the panel simple while still supporting pubkeys, PKHs, and auto-signedsigarguments.