chore(cli): fix commitment to new issue #33
Conversation
sphamjoli
requested review from
andrew-ifrita,
corey-hathaway,
filvecchiato and
waytwotall
as code owners
CI Summary
Release - PassedTest this PR Download artifact (GitHub CLI required): gh run download 23412528320 -n cli-preview-0.0.0-pr.33 -R paritytech/dotns-sdkInstall globally:
Verify: dotns --helpDeploy Example — FailedFailed at: Deploy — Deploy workflow failed — see run logs for upload/register/contenthash details
Labelspkg: cli, type: test |
sphamjoli
changed the title
sphamjoli
changed the title
| ? Number(currentCommitTimestamp) | ||
| : currentCommitTimestamp; | ||
|
|
||
| const nowOnChain = Math.floor(Date.now() / 1000); |
There was a problem hiding this comment.
Why is this called nowOnChain when it is just the wall time and not a current timestamp from onchain?
packages/cli/src/utils/constants.ts
Outdated
| export const OPERATION_TIMEOUT_MILLISECONDS = 300_000; | ||
|
|
||
| export function getCommitmentBufferSeconds(): number { | ||
| return Number(process.env.DOTNS_COMMITMENT_BUFFER ?? 6); |
There was a problem hiding this comment.
We can get NaNs here which might cause some weirdness
There was a problem hiding this comment.
If a user inputs "nonsense" it becomes Number("nonesense") => NaN which will get passed into something later and break with potentially hard to track errors
| merged.__statusProvided = allOpts?.status != null; | ||
|
|
||
| if (allOpts?.commitmentBuffer != null) { | ||
| process.env[ENV.COMMITMENT_BUFFER] = String(allOpts.commitmentBuffer); |
There was a problem hiding this comment.
This is a bit weird tbh. Can't we just pass the options around instead of using ENV as a sidechannel? Doing this with env may introduce some hard to replicate bugs and also isn't fantastic for readability/understandability of the code when only looking at an isolated segment.
There was a problem hiding this comment.
@andrew-ifrita I see, your perspective we have that already do you suggest we remove setting this in an ENV?
There was a problem hiding this comment.
Yeah. It is fine to read the settings from ENV, but then we should just pass the settings object around. We shouldn't set an ENV just to read it later.
There was a problem hiding this comment.
Read from ENV but don't write to ENV and use it as a side channel
2 participants
Description
Replace the hardcoded 6-second wall-clock wait after commitment submission with on-chain polling verification. After the initial wait (minCommitmentAge + buffer), the CLI now reads the
commitments(hash)contract function to confirm the chain timestamp has actually advanced past the minimum age before attempting registration. The buffer is configurable via--commitment-buffer/--cbflag orDOTNS_COMMITMENT_BUFFERenv variable (defaults to 6 seconds).Type
Package
@dotns/cliRelated Issues
Fixes
https://github.com/paritytech/dotns/issues/38
Checklist
Code
bun run lintpassesbun run formatpassesbun run typecheckpassesDocumentation
Breaking Changes
Breaking changes:
Testing
How to test:
bun test packages/cli/tests/unit/register/registerHelp.test.ts- 22 unit tests for help text, option parsing, commitment buffer defaults, and env variable overridedotns register domain --name testname123 -k //Alice- full registration should complete without CommitmentTooNewdotns register domain --name testname456 -k //Alice --cb 12- verify custom buffer is applied (spinner shows longer wait)DOTNS_COMMITMENT_BUFFER=15 dotns register domain --name testname789 -k //Alice- verify env variable overridedotns register domain --help- verify--commitment-bufferandDOTNS_COMMITMENT_BUFFERappear in help outputNotes