fix: hard coded TLD - #261
Conversation
…ed .dot
The SDK derived every domain node and token id from a hardcoded
namehash('dot'), but the TLD is a per-deployment value fixed at
DotnsProtocolRegistry initialisation (for example 'paseo' on the Paseo
testnet). On such deployments every ownerOf/available read queried the
wrong token id, so the availability pre-check disagreed with the on-chain
register(), surfacing as NameNotAvailable(getsome) after a passing check.
- add deriveDomainNode/deriveDomainTokenId pure helpers keyed on tldNode
- add core/naming: resolve tldNode/tld once per deployment from the
controller's protocolRegistry (cached) and derive ids and display names
from it, replacing the hardcoded DOT_NODE
- make ensureDomainNotRegistered call the controller's available(label),
the exact predicate register() enforces, instead of inferring from a
swallowed ownerOf revert
- print "using dotns cli vX.Y.Z" on every operation
- make the argv-secret warning loud and yellow
- bump CLI to 0.8.3
Tests extended in the contractInteractions and registrationManifest suites.
CI Summary
Release - PassedTest this PR Download artifact (GitHub CLI required): gh run download 32233936201 -n cli-release-0.0.0-pr.261 -R paritytech/dotns-sdkInstall globally: npm install -g ./parity-dotns-cli-0.0.0-pr.261.tgzVerify: dotns --helpDeploy Example — FailedFailed at: Deploy — Deploy workflow failed — see run logs for upload/register/contenthash details
Benchmark UI - Failed (0s)
Labelspkg: cli, type: test, dependencies Test - Passed300 passed, 0 failed across 300 tests. |
re-gius
left a comment
There was a problem hiding this comment.
Please also fix this
On a non-.dot deployment, passing the fully-qualified name (e.g. alice.paseo) produces alice.paseo.paseo.li, because dotliViewUrls only strips .dot. Please pass the normalized label from the resolved result, or make dotliViewUrls TLD-aware.
| const pending: CommitmentRecord[] = []; | ||
| for (const record of records) { | ||
| const registered = await step(`Checking ${record.label}.dot`, async () => | ||
| const registered = await step(`Checking ${record.label}`, async () => |
There was a problem hiding this comment.
Similarly, here existing implementation has a record.label.domain, while we only use the record.label
There was a problem hiding this comment.
Agreed. This one has the session context, so I will format the full name here too. Thanks for spotting it.
| console.log( | ||
| chalk.gray(" • ") + | ||
| chalk.cyan((row.label + ".dot").padEnd(24)) + | ||
| chalk.cyan(row.label.padEnd(24)) + |
There was a problem hiding this comment.
Yes, missed this. It has the context as well, so I will show label.<tld>. Will fix.
| throw new Error(`Domain ${label}.dot has no owner`); | ||
| // A name is a label plus at most one TLD segment. Reject anything else here so | ||
| // clearly-invalid input fails without a chain read for the TLD. | ||
| if (/^[a-z0-9-]+(\.[a-z0-9-]+)?$/.test(input.toLowerCase())) { |
There was a problem hiding this comment.
ig this rejects any subdomains as well right? If so is this the intended scenario considering we expect recipient identifier?
There was a problem hiding this comment.
Yes, it rejects subdomains, and that is on purpose. This branch looks up a name owner via registrar.ownerOf, and only second-level names are minted there; subdomains live in the registry and never had an owner to resolve. The old code rejected them too, via isLabelLike (which forbids dots). The regex just adds the label.tld form on top.
| @@ -10,8 +11,9 @@ async function main() { | |||
| validateDomainLabel(label); | |||
|
|
|||
| const cid = process.env.DOTNS_CID ?? "bafybeigdyr...replace_me"; | |||
There was a problem hiding this comment.
Is the fallback value really required, if so in what situation?
There was a problem hiding this comment.
That is the placeholder CID, so the example runs without setting DOTNS_CID. It is not needed for correctness, you just override it with the env var. It predates this PR, so I left it as is.
| const { ctx } = await connectDotns(); | ||
|
|
||
| const name = process.env.DOTNS_NAME ?? "example.dot"; | ||
| const name = process.env.DOTNS_NAME ?? "example"; |
There was a problem hiding this comment.
This would still have the expected Tld right? "example.theTld"
There was a problem hiding this comment.
Yes. performDomainLookup takes the bare label and builds the full name from the chain TLD, so it resolves as example.<tld>. That is why I dropped the .dot from the default: it would be wrong on a non-dot chain.
| pending = fetchTldInfo(ctx).catch((error) => { | ||
| // Do not cache failures: a transient read error must not poison later calls. | ||
| tldInfoCache.delete(key); | ||
| throw error; |
There was a problem hiding this comment.
Not very imp but, is it worth retrying before we throw an error?
There was a problem hiding this comment.
There is a light retry already, just not inline: on failure the cache entry is deleted, so the next call fetches again rather than reusing a bad result. Within a single call there is no retry, and read() has its own timeout. Happy to add a small bounded retry if you would prefer one.
There was a problem hiding this comment.
Ig should be fine, that way we also defer any unwanted overheads to make the call
| "name": "@parity/dotns-cli", | ||
| "module": "index.ts", | ||
| "version": "0.6.9", | ||
| "version": "0.8.3", |
There was a problem hiding this comment.
How do we decide the version? Is this in sync with other packages??
There was a problem hiding this comment.
The version is out of sync it was an oversight for many versions
…g nodes (#277) #### Description `lookup` hashed user input as-is, so a fully-qualified name (`alice.paseo`) derived the wrong node/tokenId, reported a registered name as "not registered", and displayed a doubled suffix (`alice.paseo.paseo`) — while every write path (register, transfer, delegate, escrow, records) already normalised input through `normaliseName` (chain-resolved TLD, from #261). This PR normalises both lookup entry points the same way: - `performDomainLookup` — normalises before `formatDomainName`/`domainNode` (also fixes the trailing-digit base-name check running on the suffixed string) - `performOwnerOfLookup` — normalises instead of only trimming - `lookup transfer` — normalises its display label and JSON `label` output (the underlying `transferName`/`verifyDomainOwnership` already normalised) #### Testing How to test: 1. `bun test packages/cli/tests/unit/lookup/` — new tests pin that `alice` and `alice.<tld>` derive the same node/tokenId against a mocked chain, and that a foreign suffix (`alice.dot` on a `.paseo` deployment) is not stripped. 2. Live: `bun run src/cli/index.ts lookup owner-of <name>.paseo --env paseo-v2` — now prints `Domain: <name>.paseo` (was `<name>.paseo.paseo`) and reads the correct token. All 304 unit tests pass. #### Notes Verified on paseo-v2 (previewnet's shared dev account currently has no balance/mapping after the chain relaunch, so it cannot serve as a live testbed until refunded). --------- Co-authored-by: Giuseppe Re <giuseppe.re@parity.io>
Description
The SDK derived every domain node and token id from a hardcoded
namehash('dot'), but the TLD is a per-deployment value fixed atDotnsProtocolRegistryinitialisation (for example,paseoon the Paseo testnet, whosetld()returns.paseo). On such deployments, everyownerOf/available/subnode read queried the wrong token id, so the availability pre-check disagreed with the on-chainregister(), surfacing asNameNotAvailable(getsome)after a passing check.This PR resolves the TLD from chain across the whole CLI, so no code path assumes
.dot, and adds a pre-commit gate against a related code-quality issue (dangling inline types).Core resolution
deriveDomainNode/deriveDomainTokenIdpure helpers keyed ontldNodecore/naming: resolvetldNode/tldonce per deployment from the controller'sprotocolRegistry(cached), and derive ids, nodes and display names from it, replacing the hardcodedDOT_NODE(now removed).resolveTldInfostrips the registry's leading-dot suffix (.paseobecomespaseo)ensureDomainNotRegisteredcall the controller'savailable(label), the exact predicateregister()enforces, instead of inferring from a swallowedownerOfrevertFull TLD sweep
domainNode(ctx, parent)instead ofnamehash(\${parent}.dot`)`performDomainLookupderives the node and display name from the chain TLDnormaliseLabel(name, tld)strips the deployment's TLD suffix, which is what distinguishes a second-level name (alice.paseo) from a subdomain (sub.alice); a newnormaliseName(ctx, name)resolves the TLD and applies itformatDomainName(ctx, label)rather than a hardcoded.dot; cross-environment cache listings show the bare labelErgonomics
using dotns cli vX.Y.Zon every operationTooling
scripts/checkDanglingTypes.mjs: an AST gate that blocks new inline object shapes in function signatures, generics and type assertions. Wired into the pre-commit hook in diff-scoped (--staged) mode so it only fails on newly introduced ones, and exposed asbun run check:types/check:types:stagedTests
validation,contractInteractionsandregistrationManifestsuites (no new test files)DOTNS_ENV) via a cached helper, so assertions never check against the wrong TLDType
Package
@parity/dotns-cli.githooks/pre-commit,scripts/checkDanglingTypes.mjs)Related Issues
Fixes
#225
Registrations and lookups on non-
.dotdeployments (paseo-v2, previewnet) behaving incorrectly because the SDK assumed a.dotTLD: registration failing withNameNotAvailableafter a passing availability pre-check, subname parent nodes computed under the wrong TLD, and names displayed with the wrong suffix.Checklist
Code
bun run lintpassesbun run formatpassesbun run typecheckpassesDocumentation
Breaking Changes
computeDomainTokenIdmoved fromutils/contractInteractionstocore/namingand is now asynchronous and context-aware, andnormaliseLabelgained an optionaltldargument (defaulting todot). Both are internal helpers, not part of the published package surface, so consumers are unaffected.Breaking changes:
None.
Testing
How to test:
cd packages/cli && bun run typecheck && bun run lint && bun test tests/unit/(298 unit tests pass)..dotdeployment:bun run src/cli/index.ts lookup owner-of getsome --env paseo-v2reportsRegistered: trueunder the real TLD, whereas before the change it wrongly reportednot registered.bun run src/cli/index.ts lookup name getsome --env paseo-v2showsdomain: getsome.paseo(single dot), notgetsome.dot.DOTNS_MNEMONIC=... bun run src/cli/index.ts register domain --name <freeLabel> --env paseo-v2completes through commit-reveal instead of revertingNameNotAvailable.using dotns cli v0.8.3, and that--mnemonicprints a loud yellow security warning.bun run check:typeslists existing debt;bun run check:types:stagedblocks a commit that stages a new inline object shape in a signature, generic or assertion, and passes otherwise.Notes
siphamandlaregistered successfully (tx0xc5e78acb1d87399e6ff03e5b7df6971a7f6c3cd3d05c6a833ea14ee530b8db64, 0 PAS), andgetsomewas confirmed already registered under.paseo, which was the original failure.DotnsProtocolRegistry.tldNode()is immutable after initialisation.