You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Rewrites wallet-integration on @icp-sdk/signer and narrows it to integrating a signer. @dfinity/oisy-wallet-signer is dropped.
Closes#364 — the last skill still anchored to an incompatible @icp-sdk/core major. Install lines assume the ^6 baseline from #400; the two PRs touch disjoint files and merge in either order, but #400 first avoids a window where this skill says ^6 and its neighbours say ^5.
Reviewed by @sea-snake, who maintains @icp-sdk/signer — his changes are in, across two rounds. The second round: SignerAgent is now recommended over calling callCanister directly rather than left to inference, and the redirect example's URL no longer implies ICRC-167 dictates a path (/sign, not /icrc-167).
Pitfall 9 also now covers what declaring a callbackUrl does not buy you: the wallet reads /.well-known/ii-auth-callbacks cross-origin, so the document needs a JSON response and CORS headers or a correctly listed callback still fails validation. Raised by Copilot, confirmed by the maintainer, and documented the same way the internet-identity skill documents the same file.
Scope
@icp-sdk/signer is the relying-party client, so the skill covers the app side only. Deleted: the Wallet Side (Signer) section, Signer.init prompt registration, the ICRC-21 consent-message state machine, and the pseudo-wallet dev setup. Consent rendering and account custody are the wallet's job.
The model is one thing: every write is an individual, user-approved ICRC-49 call. Wanting a session instead means wanting authentication, so that is an explicit When NOT to use pointing at internet-identity. ICRC-34 is not covered — per the maintainer, a delegation is scoped and issued for auth purposes, not for wallet purposes with explicit approval. That restores the boundary the old skill drew, with the right reason: ICRC-34 exists, it just is not a wallet mechanism.
Generic over ICRC-25 signers with OISY as the worked example. For another web signer the transport URL is usually the only change, and BrowserExtensionTransport.discover() finds extension signers you never hardcoded — but which transport reaches a signer is a separate question from what that signer supports, so the skill negotiates rather than assumes. Standards covered: ICRC-25, 27, 29, 49, 94, 167.
Structure
Transport choice is the one decision up front — PostMessageTransport (ICRC-29 popup), UrlTransport (ICRC-167 redirect, new in signer 6), BrowserExtensionTransport (ICRC-94 discovery) — then capability negotiation, the permission and account lifecycle, and calls through SignerAgent.
IcpWallet/IcrcWallet are replaced by SignerAgent + a ledger client from @icp-sdk/canisters@^4, which needed the canisters 4.0.0 release to be installable alongside signer 6.
Claims corrected rather than carried over
"Concurrent requests return 503 BUSY." No such code in ICRC-25, and the library does not serialize — UrlFlow deliberately batches concurrent requests. It was an oisy extension. The old error table mixed five vendor codes in with the standard ones.
Reads through SignerAgent "cost cycles." They do not; the cost is the user's approval interaction. Public data needs no wallet at all — read a ledger balance with an ordinary HttpAgent, anonymous by default.
The error taxonomy was wrong in both directions.SignerAgentError is not a transport failure — it fires when the wallet did respond and the response failed validation, so "reconnect and retry" was the wrong advice. And transport failures do not arrive as PostMessageTransportError at all: Signer.openChannel() rethrows them as SignerError with code 4000, original as cause. Runtime-verified against a transport that fails to establish:
The handler narrows on err.cause, then falls back by range — ICRC-25 owns 1xxx/2xxx/3xxx/4xxx and names only a few codes inside each, so a signer may return 3002 and the old default: throw would have mishandled it. The library itself emits only 1000 and 4000; 4001 reaches you only if the signer returns it. An unnamed 3xxx reports that the action did not go through rather than returning silently — 3001 is the only code where silence is right, because there you know the user cancelled on purpose.
The redirect example returned unverified wallet output.callCanister validates only that contentMap and certificate are present and decodable; the content-map match and the certificate check live exclusively in SignerAgent. The maintainer confirmed SignerAgent works over UrlTransport, so the example now uses it and the raw primitive is gone from the skill — which removes the problem rather than warning about it.
The memoize() rule was too strict. The skill said it "is the only place a flow may await anything that is not a signer request", taken from the UrlTransport docstring. Per the maintainer the journal cares whether a value comes back identical, not whether an await happened, so deterministic async work needs no memoize. Corrected, and flagged upstream since the docstring wording is what produced the error.
getAccounts() was indexed blindly. ICRC-27 defines accounts as a vec with no minimum and lets the signer prompt the user to choose which to share, so an empty list means declined, not failed, and several means the user should pick. return accounts[0] gave undefined on an empty list, crashing later at .owner. connect() returns the list; pitfall 4 covers both cases.
Account identity — one rule
Every account-shaped value is an IcrcAccount (exactly what getAccounts() returns and what balance() takes), converted at the ledger boundary only:
Comparing encodings normalizes by construction — absent, undefined and 32 zero bytes all encode to the bare principal, a real subaccount does not — which is what the reconnect check needs; comparing owner alone accepts a stale selection. SignerAgent is the documented exception: its account is a Principal and cannot carry a subaccount.
The subaccount is usually absent, because signers commonly offer only the default one. Carrying the account whole costs nothing, so the skill does, without implying subaccounts are the common case.
Verified
Every block compiles against @icp-sdk/signer@6.0.0 + @icp-sdk/core@6.1.0 + @icp-sdk/canisters@4.0.0, strict, skipLibCheck: false, two ways: merged (whole document as one module — catches a block referencing anything the skill never defines) and isolated (each block alone — catches a block relying on another block's imports). The merged pass found an example reaching into another function's scope, three blocks using top-level await, and one shadowing account; the isolated pass found nine blocks short of their own imports. Neither can see an unreachable branch, which is how the error-handling bug above survived them.
Prose is checked against the code too, not only the code against the library. Twelve defects here were a claim contradicting, or looser than, guidance elsewhere in the same file; for a skill the prose is the payload. Every API name in prose resolves against the libraries and the skill's own blocks (55 identifiers, 0 unresolved), and every prose assertion about an API is enumerated for review (24) — the latter caught the testing section crediting localhost as a secure context, when isSecureContextUrl is applied to the signer's URL, not the relying party's origin.
One runtime bug caught by compiling: an early redirect example journaled a CryptoKey-backed identity through memoize(). It compiles and fails at runtime, because memoize persists via JSON. Pitfall 6 now documents that trap generally.
Patterns cross-checked against hosting/oisy-signer-demo, already on @icp-sdk/signer (5.3.0). Two things it taught the skill: reads belong on a plain HttpAgent, and a connection cannot survive a reload — persist the account, render read-only, reconnect lazily on first write. That demo is a candidate for its own bump to signer 6 / core 6 / canisters 4.
Not covered
The skill tells the agent to call getSupportedStandards() rather than asserting which standards OISY advertises — a specific wallet's capability set cannot be verified from here, and negotiating is the right instruction for a generic skill anyway.
Eval results — full replacement, 10 cases with baseline
All four previous cases tested oisy specifics (IcpWallet vs IcrcWallet, signer-side implementation) and no longer described the skill.
Adversarial: reaches for the superseded oisy library WITH 3/3 | WITHOUT 0/3
Adversarial: reading a balance through SignerAgent WITH 3/3 | WITHOUT 1/3
Adversarial: establishing the wallet popup on mount WITH 3/3 | WITHOUT 1/3
Adversarial: signer 6 against a core ^5 project WITH 3/3 | WITHOUT 2/3
Adversarial: assumes every wallet can do what the app … WITH 3/3 | WITHOUT 1/3
Adversarial: getAccounts() returns a list, not an account WITH 3/3 | WITHOUT 0/3
Connection does not survive a page reload WITH 4/4 | WITHOUT 2/4
Adversarial: a blocked popup is not the error class … WITH 4/4 | WITHOUT 0/4
Adversarial: an ICRC-25 code the table does not name WITH 3/3 | WITHOUT 0/3
Adversarial: redirect flow loses a value across the nav WITH 2/2 | WITHOUT 0/2
TOTAL WITH 31/31 | WITHOUT 7/31
Trigger evals: should-trigger 6/6, should-not-trigger 7/7 — including three boundary cases that must not match: "I'm building a wallet — how do I handle incoming ICRC-49 call requests from dapps?" (signer implementation), "Log my CLI agent into oisy.com so it can act as me" (→ agent-web-identity), "Add Internet Identity login to my app" (→ internet-identity).
Latest run per case; cases were re-run as content changed, so the baseline column is not from one sitting.
A delegation case was removed along with ICRC-34. Four were added for behaviour that had landed uncovered: getAccounts() returning a list, an ICRC-25 code outside the named set, a redirect value lost across the navigation, and capability negotiation.
The blocked-popup case is flaky — 2/4, 4/4, 4/4, 3/4 across four runs. The failures are legitimate rather than mis-scored: the model sometimes answers narrowly, splitting 4000 by err.cause for the popup case but rethrowing the rest, so a 4000 with no cause goes unhandled. Baseline is 0/4 every time, so the delta is stable even where the absolute is not.
Six expectations were encoding my own errors or over-reaching past their prompt, and were corrected — I was writing them from the skill's content rather than from what a correct answer to that exact prompt would contain. The suite caught each one: one asserted reads "cost cycles"; one demanded 4000 route to reconnect when the skill deliberately splits it by err.cause; one asked a "short answer" prompt to enumerate all four ICRC-25 ranges.
Replaces the @dfinity/oisy-wallet-signer material with @icp-sdk/signer,
the relying-party client, and narrows the skill to integrating a signer.
The "Wallet Side (Signer)" section, prompt registration and the
ICRC-21 consent-message machinery are gone -- that is implementing a
wallet, and consent rendering belongs to the wallet.
Generic over ICRC-25 signers with OISY as the worked example: transport
choice (ICRC-29 popup, ICRC-167 redirect, ICRC-94 extension discovery),
capability negotiation, permissions and accounts, then the two
interaction models -- per-action approval via SignerAgent (ICRC-49) and
session delegation (ICRC-34).
Two claims from the old skill are retracted rather than ported:
- "concurrent requests return 503 BUSY" -- no such code in ICRC-25, and
the library does not serialize. It was an oisy-only extension.
- "not a session system / no ICRC-34" -- requestDelegation exists, so
the premise the old When-NOT-to-Use section rested on is false.
Every API call in the file typechecks against @icp-sdk/signer 6.0.0,
@icp-sdk/core 6.1.0 and @icp-sdk/canisters 4.0.0 under strict with
skipLibCheck disabled.
All four previous cases tested @dfinity/oisy-wallet-signer specifics
(IcpWallet vs IcrcWallet, signer-side implementation) and no longer
describe the skill. Six cases replace them, weighted toward what an
agent gets wrong unaided: library choice, reads through SignerAgent,
popup on mount, interaction-model selection, the canisters ^3/^4 split,
and reconnect-after-reload.
…not general knowledge
The interaction-model case scored 3/3 both with and without the skill --
the model already knows frequent writes want a delegation, so it was a
regression net for general knowledge. Retargeted at what only the
library can tell you: requestDelegation validates the wallet's response
and throws, so callers must not hand-roll chain verification.
Same error as the one Copilot caught on #400: for a PEER conflict the
flag does not install two copies of core. Verified -- core@^5 + auth@^10
under that flag installs one core (5.4.0) beside auth 10.0.0, an
incompatible pair. It skips the check, so the mismatch shows up at
runtime rather than at install time.
Copilot found the read/write example referencing `agent` and
`signerAgent`, both local to the `transfer` function in the block above
-- so copying it yields undefined identifiers. Path A is now one
`connectLedger` helper returning both ledger clients, with usage in a
second self-contained function.
Checking for that class of defect systematically turned up three more:
- Three blocks used top-level `await`, which the skill's own pitfall 10
tells readers to avoid (and Vite's default es2020 target rejects).
All are now wrapped in functions.
- The reload block used `account` before declaring a different
`account` in the same block. Split into rememberAccount /
restoreAccount.
Verified by compiling the whole document as one module with imports
merged and only genuinely external functions stubbed -- so a block
referencing anything the skill never defines now fails the check. The
earlier harness passed those identifiers in as parameters, which is
why it proved the API calls real but not the blocks copy-pasteable.
Trimmed the transport table's redundant Standard column to stay under
the 5000-token body recommendation.
Copilot's latest review on #401 lists no findings, but its summary line
names "account/subaccount preservation". That pointed at a real defect:
connect() surfaced `subaccount` from getAccounts() and nothing used it,
while the examples hardcoded `subaccount: []` and omitted
`from_subaccount`. An account with a subaccount is a different account,
so a wallet handing one back would have had its balance read from one
place and its tokens spent from another, silently.
SignerAgent has no subaccount field -- `account` is a Principal -- so
the subaccount has to travel in the ledger call arguments instead.
connect() now returns the account whole, connectLedger takes an
IcrcAccount (which is exactly getAccounts()' element shape), balance()
receives it entire, and transfer() sets from_subaccount. Pitfall 12
records the constraint.
Caught one more mismatch on the way: safeTransfer still declared
account as Principal after its callee moved to IcrcAccount. The
whole-document compile flagged it, which is what that check is for.
Latest review reports Findings: None and shows the one earlier thread as resolved — but the overview line says "Fix account/subaccount preservation and validation issues in the integration examples." No inline comment rendered for it, so I went looking, and it was pointing at something real. Fixed in the latest commit.
The defect.connect() surfaced subaccount from getAccounts() and then nothing used it. The examples hardcoded subaccount: [] on the to account and omitted from_subaccount entirely. An ICRC-1 account with a subaccount is a different account, so for any wallet handing one back, the skill would have had you read the balance of one account and spend from another — with no error either side.
Why it was easy to get wrong, and now a pitfall: SignerAgent has no subaccount field. Its account is a Principal, and replaceAccount(account: Principal) likewise — the agent routes calls as a principal. So the subaccount cannot ride on the agent; it has to travel in the ledger call arguments:
asyncfunctionconnectLedger(signer: Signer,account: IcrcAccount){// SignerAgent routes calls as a principal; it has no subaccount field.constsignerAgent=awaitSignerAgent.create({ signer,account: account.owner, agent });
...
}constbalance=awaitread.balance(account);// { owner, subaccount? } — pass it wholeconstblock=awaitwrite.transfer({to: {owner: to,subaccount: []},from_subaccount: account.subaccount,// or the ledger spends the default
amount
});
BalanceParams = IcrcAccount & QueryParams and TransferParams.from_subaccount?: Subaccount, and getAccounts() returns { owner: Principal, subaccount?: Uint8Array } — which isIcrcAccount, so connect() now just returns the element whole instead of splitting it into two fields that invite dropping one.
The whole-document compile then caught a follow-on: safeTransfer still declared account: Principal after its callee moved to IcrcAccount. That is the check earning its keep for the second time.
Eval 2 ("reading a balance through SignerAgent"), which covers this content, re-run with baseline: WITH 3/3 | WITHOUT 1/3, unchanged.
A process note, since this is the third time. The overview prose and the findings list have disagreed on all three reviews of this pair — "three moderate issues" with one listed, "9 out of 9 changed files" over a stale six-file table, and now a named issue class with Findings: None. Each time the prose was pointing at something real. I am treating the overview text as a lead worth chasing rather than noise, but it does mean the rendered findings are not a complete list, and a human reviewer skimming only the Open section would have missed this one entirely.
The reload recipe stored only the owner principal, which contradicted
pitfall 12 two sections earlier: after a refresh it could read only the
owner's default subaccount, so the balance shown could differ from the
connected account and later writes could not set the original
subaccount. Valid finding from Copilot on #401.
Now stores the ICRC-1 textual encoding via encodeIcrcAccount, which
round-trips owner and subaccount as one string, and decodes on restore
with a catch that clears a stale or malformed value. ensureSignerAgent
takes the IcrcAccount and narrows to .owner itself, so callers never
juggle the two shapes.
Eval 6's expectation said "the account principal"; generalised to "the
account" now that both halves are persisted.
The previous commit fixed the recipe to persist the whole IcrcAccount
but left two prose lines telling readers to persist only the principal
-- the section intro above the recipe, and pitfall 3. Following either
would have reintroduced exactly the bug pitfall 12 warns about, and
prose is what an agent reads when it does not copy the block verbatim.
Valid finding from Copilot on #401, which also spotted the second
occurrence.
This helper unconditionally requests the ICRC-49 permission, even though the section immediately above says a signer may support ICRC-34 without ICRC-49 and Path B is a supported use case. On a delegation-only signer, connect() therefore fails with 2000 before getAccounts() can run. Either omit this optional pre-request (the documented ask_on_use default is sufficient) or make it conditional on the negotiated capabilities and request only the methods needed by the selected path.
verify a restored account is still offered
Copilot's stated finding on #401 -- that connect() fails with 2000 on a
delegation-only signer -- is wrong: ICRC-25 requires a signer to ignore
scopes it does not support ("proceed as if the scopes array did not
include that object"), and 2000 is not a declared error for
icrc25_request_permissions. No failure to fix.
Two smaller points underneath it are real and taken:
- connect() hardcoded the ICRC-49 scope while sitting in a section that
serves both paths, so a Path B app asked for a permission it never
exercises. It now takes the scopes as a parameter, with the two paths'
sets shown above it.
- ensureSignerAgent trusted the restored account. It already called
getAccounts() to re-establish the channel, so it now checks the stored
account is still among those offered and clears it if the user switched
accounts in the wallet while the page was gone.
Latest review: Findings: None, the prose thread confirmed resolved, and one Previously missed item at :171 — no thread attached, so recording here.
The stated finding is wrong
On a delegation-only signer, connect() therefore fails with 2000 before getAccounts() can run.
ICRC-25 forbids exactly that. From the spec's icrc25_request_permissions section:
scopes: Array of permission scope objects the relying party requires. If the signer does not support a requested scope, it should ignore that particular scope and proceed as if the scopes array did not include that object.
and the processing steps:
The signer removes any unrecognized scopes from the array of requested scopes.
The declared errors for that method are only 1000 Generic error and 3000 Permission not granted — 2000 is not among them. A delegation-only signer drops the ICRC-49 scope and answers with the scopes it does support; getAccounts() is unaffected. So there is no failure to fix, and no 2000 to guard against.
Two smaller points underneath it are real, and taken
Least privilege.connect() hardcoded the ICRC-49 scope while sitting in a section that serves both paths, so a Path B app would ask for a permission it never exercises. Not a failure, but it puts a permission in front of the user for nothing. It now takes the scopes:
The comment now also states the rule the finding got backwards — that over-asking is ignored rather than fatal — since that is the non-obvious part worth writing down.
Stale accounts, which the overview line also named. ensureSignerAgent trusted the restored account, but the user may have switched accounts in the wallet while the page was gone. It already called getAccounts() to re-establish the channel, so the check is nearly free:
constoffered=awaitsigner.getAccounts();consttext=account.owner.toText();if(!offered.some(({ owner })=>owner.toText()===text)){sessionStorage.removeItem(SESSION_KEY);thrownewError('the wallet no longer offers the stored account; reconnect');}
I deliberately did not attach an error code to that case: ICRC-49 says the signer displays sender to the user and rejects what it cannot complete, but it does not define a specific code for an unheld sender, so asserting one would be inventing it.
Whole-document compile still clean. Eval 6 re-run with baseline: WITH 4/4 | WITHOUT 1/4.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
Transport errors, redirect callback setup, and prompt-count behavior need correction.
Review effort: Balanced Findings: None
Previously missed (3)
In code that hasn't changed since last review
Avoid mislabeling transport failures as blocked popups
skills/wallet-integration/SKILL.md:351
PostMessageTransportError identifies the transport, not specifically a blocked popup. The same transport class can be the cause when an opened channel closes or otherwise fails (the text below explicitly notes that channel closure is also reported as 4000), so this branch can show popup-blocked instructions for a real disconnect. Use generic postMessage transport help here, or inspect a reason/code that specifically denotes establishment being blocked before selecting that message.
Avoid asserting exactly one prompt for every transfer
skills/wallet-integration/SKILL.md:447
A transfer is not guaranteed to produce exactly one prompt. As explained in the permissions section, the initial icrc49_call_canister permission state is signer-defined; when it is ask_on_use, the first transfer may require a permission interaction in addition to the per-call approval. Avoid asserting a fixed prompt count for a generic ICRC-25 signer.
Document required callback validation response and CORS headers
skills/wallet-integration/SKILL.md:416
Declaring the callback URL is not sufficient for browser-based validation: the wallet reads /.well-known/ii-auth-callbacks cross-origin, so the document also needs a JSON response and CORS headers. Without that setup the correctly listed callback still fails validation. Include the response shape and headers here, as the repository's internet-identity skill does at lines 297-315.
- The redirect example's URL was https://wallet.example.com/icrc-167,
which implies ICRC-167 dictates a path. It does not; the path is the
wallet's own. Now /sign, with a comment saying so.
- Recommend SignerAgent over calling callCanister directly, explicitly
rather than by implication. The note already said SignerAgent does the
content-map and certificate checks that callCanister does not; it now
leads with "prefer SignerAgent unless you have a reason not to" and
says the example uses the primitive only because the redirect
transport makes the request shape easier to see.
- Declaring the callbackUrl is not sufficient: the wallet reads
/.well-known/ii-auth-callbacks cross-origin, so it needs a JSON
response and CORS headers or a correctly listed callback still fails
validation. Pitfall 9 now carries the document shape and the _headers
block, matching how the internet-identity skill documents the same
file. (Copilot raised this; sea-snake confirmed it.)
Copilot's other two findings are not applied, per sea-snake: the
transport-error branch is correct as written, and the "exactly one
prompt" claim was a hallucination.
…gent
sea-snake confirmed SignerAgent is designed to work with UrlTransport,
and that the awaits I was worried about are deterministic and therefore
harmless. That removes the only reason the skill showed a raw
callCanister, so the redirect example is now the same shape as the popup
one -- SignerAgent driving an IcrcLedgerCanister transfer -- and gets the
content-map and certificate checks instead of handing unverified output
back to the caller.
This closes three things at their source rather than by caveat: his
"prefer SignerAgent" point, Copilot's finding that the example returned
unverified wallet output, and my own open question about whether
ICRC-167 plus ICRC-49 was even a real pattern.
It also corrects a rule the skill had stated too absolutely, taken from
the UrlTransport docstring: "memoize() is the only place a flow may
await anything that is not a signer request". Per the maintainer that is
stricter than reality -- the journal cares whether a value comes back
identical, not whether an await happened. So rule 2 is now "put anything
that must come back the same value through memoize()", with the note
that deterministic async work such as building an HttpAgent needs none.
The standalone callCanister warning is folded into one sentence where
the choice is actually made, and the standards table leads with
SignerAgent. Eval 9's first behaviour encoded the absolute rule and is
corrected: WITH 2/2 | WITHOUT 0/2.
@sea-snake that settles it — thank you, and it removed a section rather than adding a caveat, which is the better outcome.
The redirect example now uses SignerAgent, so it is the same shape as the popup flow and gets the content-map and certificate checks instead of handing raw { contentMap, certificate } back to the caller:
asyncfunctiontransferOverRedirect(account: IcrcAccount,to: IcrcAccount,amount: bigint,ledgerId: Principal){constsigner=newSigner({ transport });// Deterministic, so no memoize needed — the same agent is built on each load.constagent=awaitHttpAgent.create({host: 'https://icp-api.io'});constsignerAgent=awaitSignerAgent.create({ signer,account: account.owner, agent });constledger=IcrcLedgerCanister.create({agent: signerAgent,canisterId: ledgerId});returnledger.transfer({to: toCandidAccount(to),from_subaccount: account.subaccount,
amount
});}
That closes three separate things at their source: your "prefer SignerAgent" point, Copilot's finding that the example returned unverified output, and my own open question about whether ICRC-167 plus ICRC-49 was even a real pattern. The standalone callCanister warning is now one sentence where the choice is actually made, and the standards table leads with SignerAgent.
Your second remark fixed a rule I had stated too strictly. I had taken this from the UrlTransport docstring:
memoize … the sole place a flow may await non-request async.
and written it as "memoize() is the only place a flow may await anything that is not a signer request". Per your point the journal cares whether a value comes back identical, not whether an await happened — so a deterministic await is harmless. Rule 2 is now:
Put anything that must come back the same value through memoize() — a nonce above all. Deterministic async work needs no memoize: building an HttpAgent yields an equivalent agent on every load, so it cannot drift from the journal.
Worth flagging for the library: that docstring line is what led me wrong, and it would lead an agent the same way, since it reads as a hard constraint on await rather than a constraint on value stability. If the wording there matched what you just told me, the skill would not have needed correcting. Happy to open an issue on icp-js-signer if useful.
One eval had encoded the strict version and is corrected — WITH 2/2 | WITHOUT 0/2. Merged and isolated compiles clean, validate 31/31.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
The top-level-await guidance contradicts the required redirect replay lifecycle.
Review effort: Balanced Findings: None
Previously missed (1)
In code that hasn't changed since last review
Do not require UI gestures for UrlTransport redirect replay
skills/wallet-integration/SKILL.md:430
This rule contradicts the redirect recipe at lines 118–119, which must restart on callback-route load so UrlTransport can replay its journal. Requiring every wallet call to originate in a UI handler would force a second click after each redirect and break replay. Restrict the user-gesture rule to PostMessageTransport; UrlTransport is the explicit exception.
…t's, not every transport's
Pitfall 10 said a module-load await "fires a wallet request outside a
user gesture" and to wrap calls in functions the UI invokes. That is
right for PostMessageTransport and backwards for UrlTransport, whose
flow must re-run on the callback-route load so the journal can replay --
so the pitfall contradicted the redirect recipe eighteen lines of code
away, and following it would have required a second click after every
redirect and broken replay.
Verified the asymmetry rather than assuming it: PostMessageTransport has
detectNonClickEstablishment (default true, postMessageTransport.js:67),
UrlTransport has no gesture check at all and navigates via
location.assign (urlFlow.js:261), which browsers do not gate on user
activation the way they gate popups.
Pitfall 1 was already correctly scoped to PostMessageTransport; only
pitfall 10 generalised. It now names UrlTransport as the exception and
says why a click there would break the flow.
Copilot's finding. sea-snake has not weighed in on this one, but the
contradiction is checkable from the skill plus the transport option
surface, so I have not held it for him.
Pitfall 10 said a module-load await"fires a wallet request outside a user gesture … wrap calls in functions the UI invokes." True for PostMessageTransport, backwards for UrlTransport, whose flow must re-run on the callback-route load so the journal can replay. So the pitfall contradicted the redirect recipe eighteen lines of code away, and following it would have required a second click after every redirect and broken the flow.
I verified the asymmetry rather than taking it on the review's word:
PostMessageTransport detectNonClickEstablishment: true (postMessageTransport.js:67)
UrlTransport no gesture check; location.assign (urlFlow.js:261)
Browsers gate popups on user activation; they do not gate a top-level navigation the same way. So the gesture requirement belongs to one transport, not to wallet calls in general.
One correction to the finding's scope: pitfall 1 was already correctly scoped — it names PostMessageTransport and detectNonClickEstablishment explicitly. Only pitfall 10 generalised, so that is the only place changed. It now reads:
…with PostMessageTransport a module-load await opens the popup outside a user gesture, which the transport rejects — pitfall 1. Wrap those calls in functions the UI invokes. UrlTransport is the exception, and the reverse: it navigates the top level rather than opening a popup, has no gesture check, and its flow must re-run on the callback-route load so the journal can replay — requiring a click there would break it.
@sea-snake you have not weighed in on this one; I did not hold it, since the contradiction is checkable from the skill plus the transport option surface rather than needing a judgement call. Flagging in case you would put it differently.
No code blocks changed, so both compiles are unaffected and still clean; validate 31/31.
For the record, this is the eleventh defect in this PR of the same shape: a rule stated correctly in one place and contradicted by an example or a neighbouring rule elsewhere. Every one compiled. It is the class my tooling cannot see, and the reason the subject-by-subject sweep is now part of the checks rather than an afterthought.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
The documented prompt count is inaccurate, and two newly added pitfalls lack required regression evaluations.
Review effort: Balanced Findings: None
Previously missed (2)
In code that hasn't changed since last review
Add eval cases for capability negotiation and post-connect writes
evaluations/wallet-integration.json:86
The rewrite adds numbered pitfalls for capability negotiation (pitfall 5) and avoiding an immediate write after connect (pitfall 13), but no output eval exercises either behavior. The repository’s skill-improvement guidance requires every newly added pitfall to receive a case; the existing mount-time popup case only covers channel establishment, not dispatching a write after a successful connection. Add focused eval cases for these two regressions.
Avoid asserting exact prompt counts without established permission scope
skills/wallet-integration/SKILL.md:459
This “prompts once” guarantee conflicts with the permission lifecycle above: when icrc49_call_canister is still ask_on_use, the first transfer can require a permission prompt as well as the per-call approval. Avoid asserting an exact prompt count unless the example first establishes that the scope is already granted.
Copilot asked for cases on two pitfalls it called newly added. They were
not new -- both were in the first commit of the rewrite (as pitfalls 4
and 12) and were only renumbered, so the "policy requires a case for new
pitfalls" premise does not apply. But the suite at 9 was thin for this
repo (icp-cli and writing-motoko carry 29), so the capability one is
worth having on its merits: an agent told "support any wallet, not just
OISY" will skip getSupportedStandards() and assume every signer does
everything.
WITH 3/3 | WITHOUT 1/3.
Not adding one for pitfall 13 ("don't fire a call immediately after
connect"): it is a UX convention a model states unprompted, so a case
would show ~no with/without delta -- the same reason the delegation
case was dropped earlier. Coverage for coverage's sake is not worth a
token-costed regression test.
The expectation as first written asked the connect-only answer to verify
ICRC-49, which the prompt (no transfer) does not call for -- the sixth
over-scope of this PR, caught by the run. Reworded to test that the code
branches on the returned standards at all, which is the durable point.
…Behavior
Copilot has raised the exact prompt count three times. sea-snake called
the mechanism it invents a hallucination -- there is no separate
ask_on_use interaction stacked on the per-call approval -- and that
stands. But "prompts once" still asserts a count the skill cannot
guarantee for a generic signer: how a wallet renders approval is its UX.
Reworded to what is actually observable and durable -- the user is shown
the call to approve and the transfer resolves with a block index -- so
the bullet no longer makes a claim about count that a wallet could
violate without being wrong.
Two findings. One accepted on its merits but not on its premise; one is a third re-raise of a point the library maintainer already settled.
1. Eval coverage for pitfalls 5 and 13 — premise wrong, suggestion half right
The premise is that these are "newly added" pitfalls and the repo's guidance therefore requires a case for each. They are not new. Both were in the first commit of this rewrite, as pitfalls 4 and 12; they only moved because pitfalls were inserted above them:
$ git show cf43d77:skills/wallet-integration/SKILL.md | grep -E '^[0-9]+\. \*\*' | grep -iE 'capabilit|immediately after connect'
4. **Assuming a wallet's capabilities.** ...
12. **Firing a call immediately after connecting.** ...
So the "new pitfall ⇒ needs a case" rule does not apply here.
But one of the two is worth adding anyway, for a reason the finding does not give: at 9 cases the suite was thin for this repo, not lean.
So capability negotiation is now covered — it is load-bearing for the generic-signer posture, and an agent told "support any wallet, not just OISY" will skip getSupportedStandards() and assume every signer does everything. WITH 3/3 | WITHOUT 1/3.
I am not adding one for pitfall 13 ("don't fire a call immediately after connecting"). It is a UX convention a model states unprompted, so the case would show little or no with/without delta — the same reason the delegation case was dropped earlier in this PR. Coverage for its own sake is a token-costed regression test that proves nothing.
2. "Prompts once" — third raise, and the mechanism is still wrong
@sea-snake ruled on this: the ask_on_use interaction stacked on top of the per-call approval that this finding describes does not happen. That stands, and I am not treating the third raise as new information.
The wording did nonetheless assert a count the skill cannot guarantee for a generic signer, since how a wallet renders approval is its own UX. So the bullet now says what is observable instead:
A ledger transfer through SignerAgent shows the user the call to approve and resolves with a bigint block index.
That removes the unprovable claim without adopting the mechanism.
One thing about my own expectations
The capability case, as first written, asked a connect-only answer to verify ICRC-49 — which the prompt ("no transfer code") does not reach. The model checking ICRC-27 was right and my expectation was wrong. That is the sixth time in this PR I have written an expectation from the skill's content rather than from what a correct answer to that exact prompt would contain. Caught by the run each time, and now recorded in the PR body.
Suite: 10 cases, 31/31 with skill, 7/31 baseline; triggers 6/6 and 7/7. Body updated to match.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
The generic compatibility claim incorrectly implies every ICRC-25 signer works by changing only the URL.
Review effort: Balanced Findings: None
Previously missed (1)
In code that hasn't changed since last review
Avoid implying ICRC-25 alone guarantees URL transfer capabilities
skills/wallet-integration/SKILL.md:19
ICRC-25 conformance alone does not mean a signer supports a URL transport, accounts, or ICRC-49 calls; this conflicts with the capability warnings later in the skill. Saying any signer works by swapping the URL can lead readers to treat an extension or account-only signer as transfer-capable. Scope this claim to URL-based signers and require capability negotiation for the needed standards.
… skill delivers
"Any ICRC-25 signer works by swapping the transport URL" sat against
pitfall 5 and the Negotiate capabilities section, which both say a
signer may expose accounts without executing calls, or speak a transport
you have not built for. ICRC-25 conformance is not a guarantee of
ICRC-27, ICRC-49, or any particular transport.
Now: the URL swap is scoped to another web signer, extensions are named
as the separate transport they are, and what a signer supports is called
out as a separate question from which transport reaches it, pointing at
the section and pitfall that cover it.
Twelfth defect of this shape in the PR -- a claim in one place looser
than the guidance elsewhere -- and the last one outstanding.
The intro said "any ICRC-25 signer works by swapping the transport URL", which promises more than the skill delivers two sections later:
Negotiate capabilities — a signer may expose accounts without executing calls, or support a transport you have not built for.
Pitfall 5 — ICRC-25 conformance says nothing about ICRC-27 or ICRC-49.
Now:
For another web signer the transport URL is usually the only change, and BrowserExtensionTransport discovers extension signers you never hardcoded. What a given signer actually supports is a separate question from which transport reaches it — negotiate it rather than assuming.
That separates the two things the original sentence conflated: which transport reaches a signer and what that signer can do.
validate 31/31; no code blocks touched, so both compiles are unaffected.
This is the twelfth defect of the same shape in this PR — a claim in one place looser than, or contradicting, guidance elsewhere in the same file. Every one compiled; none was a type error. Tallying where they were caught:
Found by
Count
Automated review
7
My subject-by-subject sweep
4
The library maintainer
1
Compilers / linters
0
That last row is the useful conclusion for this repo. The merged and isolated compiles, the import checks and --noUnusedLocals all earn their keep on code, and they are structurally blind to a sentence disagreeing with the block beneath it. For a skill, where the prose is as much the payload as the code, the sweep that groups every claim by subject and reads them together is the check that actually finds these — and it is the one I would carry into the next skill rather than any individual fix here.
Moves the skills' `@icp-sdk/core` anchor from `5.x` to `6.x`. The anchor
sits wherever `@icp-sdk/auth` is, because almost every dapp
authenticates — and auth has moved.
Refs #364; does **not** close it. `wallet-integration` is still on the
old stack and is rewritten in #401, which closes the issue.
## Why now
Every package the skills install agrees on core `^6`:
`@icp-sdk/auth@10`, `@icp-sdk/signer@6`, `@icp-sdk/canisters@4`,
`@dfinity/utils@5`, and `@icp-sdk/vetkeys@0.7` (`^5 || ^6`). Verified by
install, peers honoured:
```
core@^6 + auth@^10 + signer@^6 + vetkeys@^0.7 + canisters@^4 + utils@^5 → added 26 packages ✅
```
## The one real bug
`internet-identity` Prerequisites paired an open-ended `@icp-sdk/auth
(>= 9.0.0)` with `@icp-sdk/core (>= 5.3.0)`. That floor admits auth 10,
which peers core `^6`, so the advertised combination does not install:
```
npm error Found: @icp-sdk/core@5.4.0
npm error peer @icp-sdk/core@"^6" from @icp-sdk/auth@10.0.0
```
Both majors are now pinned together, with a pitfall for the mismatch.
Everything else is a version bump.
## Verified, not assumed
- **auth 9 → 10 is a pure peer bump.** `diff -rq` over the whole
`dist/esm` type surface: no differences. The [v10
guide](https://js.icp.build/auth/latest/upgrading/v10/) agrees ("This
package's own API is unchanged"). So the II flow needed no code changes.
- **bindgen 0.4.0 output is core-6 clean.** Bindings generated from a
`.did` exercising `variant`/`record`/`blob`/`opt`/`principal`,
typechecked against core 6.1.0 under `strict` with `skipLibCheck:
false`.
- **canisters 4.0.0 changed nothing we document.** The only changed file
in its hand-written surface is `nns/types/governance_converters.d.ts`;
the rest is regenerated declarations. `AssetManager` and both ledger
clients are untouched.
- **vetkeys 0.5 → 0.7 is API-identical.** The `>= 0.7` floor is
load-bearing for a different reason: 0.5/0.6 carried core as a plain
**dependency**, so an app on core 6 got 6.1.0 at the root and 5.4.0
nested under vetkeys. 0.7 peers it.
Two claims inherited from `main` are also corrected: `@icp-sdk/core`
does have stable 4.x releases (so "starts at 5.x, no 0.x or 1.x" was
false — dropped, since an agent needs the reason to pin, not version
history), and `--legacy-peer-deps` does not duplicate core on a *peer*
conflict — it skips the check and installs the mismatched pair. The
nested-copy wording is kept only where it is accurate, on vetkeys.
## Not in scope
`wallet-integration` is left to #401 to avoid a conflict on the same
file, and is currently self-consistent. `caffeine-app` and
`certified-variables` are handled separately per maintainer. The false
`{ agent }` / `createActor` claim in `binding-generation.md` and
`dfx-migration.md` belongs with #156, since retracting it also means
changing an eval that enforces it.
## Supersedes #368
That PR pinned everything to `^5`. Carried over: the bindgen floor, the
correction that bindgen does not depend on core, and the vetkeys 0.7
bump.
<details>
<summary>Eval results — added/changed cases, with baseline</summary>
```
internet-identity 26 (new) WITH 4/4 | WITHOUT 2/4
icp-cli 6 (changed) WITH 6/6 | WITHOUT 0/6
icp-cli 15 (changed) WITH 6/6 | WITHOUT 4/6
```
The new II case puts a core-`^5` project in front of an `@icp-sdk/auth`
install; the baseline never mentions the peer conflict and "sidesteps
the issue entirely by suggesting a different, unrelated package".
**Regression check** — `internet-identity` 20, the one existing case
whose subject I touched: **3/4 with the change, 3/4 against
`origin/main`'s content** (control run). Not a regression. Its failing
behaviour varies run to run — the model invents a specific session
default rather than saying unset falls back to II's own. Pre-existing
gap, not addressed here.
`icp-cli` 15 scored 5/6 on an earlier run, failing on the `candid:` /
`didFile:` confusion of **#367**. It did not reproduce on the latest
run, which fits that issue's non-determinism. #367 stays open.
</details>
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
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.
Rewrites
wallet-integrationon@icp-sdk/signerand narrows it to integrating a signer.@dfinity/oisy-wallet-signeris dropped.Closes #364 — the last skill still anchored to an incompatible
@icp-sdk/coremajor. Install lines assume the^6baseline from #400; the two PRs touch disjoint files and merge in either order, but #400 first avoids a window where this skill says^6and its neighbours say^5.Reviewed by @sea-snake, who maintains
@icp-sdk/signer— his changes are in, across two rounds. The second round:SignerAgentis now recommended over callingcallCanisterdirectly rather than left to inference, and the redirect example's URL no longer implies ICRC-167 dictates a path (/sign, not/icrc-167).Pitfall 9 also now covers what declaring a
callbackUrldoes not buy you: the wallet reads/.well-known/ii-auth-callbackscross-origin, so the document needs a JSON response and CORS headers or a correctly listed callback still fails validation. Raised by Copilot, confirmed by the maintainer, and documented the same way the internet-identity skill documents the same file.Scope
@icp-sdk/signeris the relying-party client, so the skill covers the app side only. Deleted: theWallet Side (Signer)section,Signer.initprompt registration, the ICRC-21 consent-message state machine, and the pseudo-wallet dev setup. Consent rendering and account custody are the wallet's job.The model is one thing: every write is an individual, user-approved ICRC-49 call. Wanting a session instead means wanting authentication, so that is an explicit When NOT to use pointing at internet-identity. ICRC-34 is not covered — per the maintainer, a delegation is scoped and issued for auth purposes, not for wallet purposes with explicit approval. That restores the boundary the old skill drew, with the right reason: ICRC-34 exists, it just is not a wallet mechanism.
Generic over ICRC-25 signers with OISY as the worked example. For another web signer the transport URL is usually the only change, and
BrowserExtensionTransport.discover()finds extension signers you never hardcoded — but which transport reaches a signer is a separate question from what that signer supports, so the skill negotiates rather than assumes. Standards covered: ICRC-25, 27, 29, 49, 94, 167.Structure
Transport choice is the one decision up front —
PostMessageTransport(ICRC-29 popup),UrlTransport(ICRC-167 redirect, new in signer 6),BrowserExtensionTransport(ICRC-94 discovery) — then capability negotiation, the permission and account lifecycle, and calls throughSignerAgent.IcpWallet/IcrcWalletare replaced bySignerAgent+ a ledger client from@icp-sdk/canisters@^4, which needed the canisters 4.0.0 release to be installable alongside signer 6.Claims corrected rather than carried over
"Concurrent requests return
503 BUSY." No such code in ICRC-25, and the library does not serialize —UrlFlowdeliberately batches concurrent requests. It was an oisy extension. The old error table mixed five vendor codes in with the standard ones.Reads through
SignerAgent"cost cycles." They do not; the cost is the user's approval interaction. Public data needs no wallet at all — read a ledger balance with an ordinaryHttpAgent, anonymous by default.The error taxonomy was wrong in both directions.
SignerAgentErroris not a transport failure — it fires when the wallet did respond and the response failed validation, so "reconnect and retry" was the wrong advice. And transport failures do not arrive asPostMessageTransportErrorat all:Signer.openChannel()rethrows them asSignerErrorwith code4000, original ascause. Runtime-verified against a transport that fails to establish:The handler narrows on
err.cause, then falls back by range — ICRC-25 owns1xxx/2xxx/3xxx/4xxxand names only a few codes inside each, so a signer may return3002and the olddefault: throwwould have mishandled it. The library itself emits only1000and4000;4001reaches you only if the signer returns it. An unnamed3xxxreports that the action did not go through rather than returning silently —3001is the only code where silence is right, because there you know the user cancelled on purpose.The redirect example returned unverified wallet output.
callCanistervalidates only thatcontentMapandcertificateare present and decodable; the content-map match and the certificate check live exclusively inSignerAgent. The maintainer confirmedSignerAgentworks overUrlTransport, so the example now uses it and the raw primitive is gone from the skill — which removes the problem rather than warning about it.The
memoize()rule was too strict. The skill said it "is the only place a flow may await anything that is not a signer request", taken from theUrlTransportdocstring. Per the maintainer the journal cares whether a value comes back identical, not whether anawaithappened, so deterministic async work needs nomemoize. Corrected, and flagged upstream since the docstring wording is what produced the error.getAccounts()was indexed blindly. ICRC-27 definesaccountsas avecwith no minimum and lets the signer prompt the user to choose which to share, so an empty list means declined, not failed, and several means the user should pick.return accounts[0]gaveundefinedon an empty list, crashing later at.owner.connect()returns the list; pitfall 4 covers both cases.Account identity — one rule
Every account-shaped value is an
IcrcAccount(exactly whatgetAccounts()returns and whatbalance()takes), converted at the ledger boundary only:encodeIcrcAccount(a) === encodeIcrcAccount(b)encodeIcrcAccount()/decodeIcrcAccount()from_subaccount(sender),toCandidAccount()(recipient)Comparing encodings normalizes by construction — absent,
undefinedand 32 zero bytes all encode to the bare principal, a real subaccount does not — which is what the reconnect check needs; comparingowneralone accepts a stale selection.SignerAgentis the documented exception: itsaccountis aPrincipaland cannot carry a subaccount.The subaccount is usually absent, because signers commonly offer only the default one. Carrying the account whole costs nothing, so the skill does, without implying subaccounts are the common case.
Verified
@icp-sdk/signer@6.0.0+@icp-sdk/core@6.1.0+@icp-sdk/canisters@4.0.0,strict,skipLibCheck: false, two ways: merged (whole document as one module — catches a block referencing anything the skill never defines) and isolated (each block alone — catches a block relying on another block's imports). The merged pass found an example reaching into another function's scope, three blocks using top-levelawait, and one shadowingaccount; the isolated pass found nine blocks short of their own imports. Neither can see an unreachable branch, which is how the error-handling bug above survived them.localhostas a secure context, whenisSecureContextUrlis applied to the signer's URL, not the relying party's origin.CryptoKey-backed identity throughmemoize(). It compiles and fails at runtime, becausememoizepersists via JSON. Pitfall 6 now documents that trap generally.hosting/oisy-signer-demo, already on@icp-sdk/signer(5.3.0). Two things it taught the skill: reads belong on a plainHttpAgent, and a connection cannot survive a reload — persist the account, render read-only, reconnect lazily on first write. That demo is a candidate for its own bump to signer 6 / core 6 / canisters 4.Not covered
The skill tells the agent to call
getSupportedStandards()rather than asserting which standards OISY advertises — a specific wallet's capability set cannot be verified from here, and negotiating is the right instruction for a generic skill anyway.Eval results — full replacement, 10 cases with baseline
All four previous cases tested oisy specifics (
IcpWalletvsIcrcWallet, signer-side implementation) and no longer described the skill.Trigger evals: should-trigger 6/6, should-not-trigger 7/7 — including three boundary cases that must not match: "I'm building a wallet — how do I handle incoming ICRC-49 call requests from dapps?" (signer implementation), "Log my CLI agent into oisy.com so it can act as me" (→ agent-web-identity), "Add Internet Identity login to my app" (→ internet-identity).
Latest run per case; cases were re-run as content changed, so the baseline column is not from one sitting.
A delegation case was removed along with ICRC-34. Four were added for behaviour that had landed uncovered:
getAccounts()returning a list, an ICRC-25 code outside the named set, a redirect value lost across the navigation, and capability negotiation.The blocked-popup case is flaky — 2/4, 4/4, 4/4, 3/4 across four runs. The failures are legitimate rather than mis-scored: the model sometimes answers narrowly, splitting
4000byerr.causefor the popup case but rethrowing the rest, so a4000with no cause goes unhandled. Baseline is 0/4 every time, so the delta is stable even where the absolute is not.Six expectations were encoding my own errors or over-reaching past their prompt, and were corrected — I was writing them from the skill's content rather than from what a correct answer to that exact prompt would contain. The suite caught each one: one asserted reads "cost cycles"; one demanded
4000route to reconnect when the skill deliberately splits it byerr.cause; one asked a "short answer" prompt to enumerate all four ICRC-25 ranges.