Why LUKSO? - New Learn Section - #1384
Conversation
LUKSO automated technical review — request changesI reviewed the complete 55-file diff at The section builds successfully, but it is not technically correct enough to approve yet. The following issues should be resolved before merge. Merge blockers
Additional confirmed technical corrections
Verification
The overall direction is valuable, but the migration and authorization errors above are operationally unsafe and should be corrected before merge. |
JordyDutch
left a comment
JordyDutch
left a comment
There was a problem hiding this comment.
Check automated report above on technical accuracy
LUKSOAgent
left a comment
LUKSOAgent
left a comment
There was a problem hiding this comment.
Re-reviewed all 55 changed files on the current head, including the two follow-up commits. Formatting and the Docusaurus production build pass. The comments below are limited to high-confidence standards and migration-correctness issues. Several can result in an unusable recovery path or an unsafe migration, so I am requesting changes before merge.
|
|
||
| Safe's "3 of 5" threshold doesn't exist directly in LSP6 — controllers are individual, not aggregated by a vote. Two patterns work: | ||
|
|
||
| - **Recovery contract** — deploy a contract that enforces the threshold itself, and register that contract as the controller holding `EDITPERMISSIONS` on the profile. Day-to-day controllers handle daily operations; the recovery contract handles ownership-level changes. |
There was a problem hiding this comment.
EDITPERMISSIONS cannot install a replacement controller that has no permission entry yet. LSP6 requires ADDCONTROLLER for new controllers and EDITPERMISSIONS for changing or removing existing ones. Please grant the recovery path both permissions, or explicitly document a pre-provisioned recovery-controller flow. See the LSP6 permission definitions.
| Permission: CALL | ||
| AllowedCalls: | ||
| - target: 0x<token>, standard: LSP7, selector: transfer | ||
| Expires: app-defined session policy |
There was a problem hiding this comment.
Core LSP6 has no controller-expiry field. LSP25 validityTimestamps limits an individual relay signature, not the controller's continuing authority. If this is only app-side policy, a direct call can bypass it. Please remove the on-chain time-window claim or identify the additional on-chain verifier/module that enforces it.
|
|
||
| ### EIP-7702 set-code delegation | ||
|
|
||
| An EOA delegates to a smart-account implementation for the duration of a transaction, pairing with a paymaster for gasless flows. |
There was a problem hiding this comment.
This describes an earlier EIP-7702 design. Final EIP-7702 delegation persists until it is replaced or cleared, and a different outer transaction sender can sponsor gas without inherently requiring an ERC-4337 bundler/paymaster stack. Please align this section with the final EIP and with line 13 of this page.
|
|
||
| ## How LSP8 and LSP4 make metadata a verifiable graph | ||
|
|
||
| LSP8 assets store per-token metadata through [ERC725Y](../../../standards/erc725.md) data keys instead of a single `tokenURI` string. LSP4 defines the metadata conventions — name, symbol, JSON schema — and its `VerifiableURI` type lets a key point to an off-chain payload _with a hash_, so the chain enforces what that off-chain blob is required to contain. Apps read `getDataForTokenId(tokenId, key)` and get a typed value back directly. Dynamic updates become `setData` calls, gated by whatever account permissions the issuer wants to configure. The metadata stops being a URL to trust and becomes a typed key-value graph you can verify. |
There was a problem hiding this comment.
A VerifiableURI is client-verifiable, not chain-enforced: the chain does not fetch or validate the remote JSON. getDataForTokenId returns raw bytes, and per-token updates use setDataForTokenId, whose standard authorization is ownership-based unless an LSP6-controlled ownership setup is added. Please correct those distinctions. See LSP2 VerifiableURI and LSP8 token metadata.
| | Criterion | Ethereum L1 | Base | Polygon | Solana | LUKSO | | ||
| | ------------------- | ------------------------------------------------------------------------------------------------------- | ------------------------ | ------------------------ | --------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | Secondary liquidity | deepest | fastest-growing EVM | strong enterprise mints | deepest non-EVM | early | | ||
| | Royalty enforcement | [ERC-2981](https://eips.ethereum.org/EIPS/eip-2981) signal | signal | signal | enforced on compressed NFTs | signal via [LSP4](../../../standards/tokens/LSP4-Digital-Asset-Metadata.md) metadata — at parity with ERC-2981, marketplace-discretionary like everywhere else on the EVM | |
There was a problem hiding this comment.
LSP4 defines digital-asset metadata, but it does not define a royalty recipient/rate signal equivalent to ERC-2981. Royalty signaling belongs to LSP18. Please reference LSP18 and state its current support status instead of claiming parity through LSP4.
| | Grant call | `approve(spender, amount)` | `authorizeOperator(operator, amount, data)` | | ||
| | Common default | dApps request max-uint to skip re-prompting | still amount-scoped by convention — not fixed by LSP7 alone | | ||
| | Who can grant new approvals | anyone holding the private key, forever | only a controller whose LSP6 permissions allow calling `authorizeOperator` — revocable at the account level | | ||
| | Revoking an existing allowance | separate `approve(spender, 0)` transaction, per token | separate `revokeOperator(operator, tokenOwner)` transaction, per token — same shape as ERC-20, LSP7 doesn't change this | |
There was a problem hiding this comment.
The current LSP7 ABI has four arguments: revokeOperator(operator, tokenOwner, notify, operatorNotificationData). The two-argument call shown here does not exist. Please include notify and the notification data, for example false, "0x". See the generated LSP7 API.
|
|
||
| | Feature | ERC1155 | LSP7 + LSP8 | | ||
| | --------------------- | ---------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------- | | ||
| | Contracts per product | one | one per asset shape (typically two) | |
There was a problem hiding this comment.
This is not generally two contracts. Each distinct fungible ERC-1155 asset needs its own LSP7 contract, plus an LSP8 collection for identifiable assets; otherwise independent balances are merged into one fungible asset. Please align this table and the tradeoff section with the corrected migration guide.
|
|
||
| ## Step 5 — sunset the old contract | ||
|
|
||
| Once the new contracts are live and every integration has migrated, pause the ERC1155 contract. Keep it deployed for historical reference — don't redeploy over it. |
There was a problem hiding this comment.
ERC-1155 does not define a standard pause function, so many existing deployments cannot perform this step. Snapshotting and minting before the old supply is frozen, burned, or escrowed also leaves both assets spendable. Please make pausing conditional and document an issuer-authorized cutover such as atomic pause/burn, escrow-and-claim, or another replay-protected migration.
| | Transfer context payload | ❌ none — bolted on via wrapper contracts | ✅ native `bytes data` on every transfer | | ||
| | Authorization | `approve` / `allowance` / `transferFrom` | `authorizeOperator` — scoped, revocable, and notifies the operator | | ||
| | Metadata | `name()` / `symbol()` / `decimals()` only | ✅ unlimited [ERC725Y](../../../standards/erc725.md) key-value storage under [LSP4](../../../standards/tokens/LSP4-Digital-Asset-Metadata.md) | | ||
| | Accidental-transfer protection | ❌ none | ✅ `force` flag rejects transfers to non-receiving contracts by default | |
There was a problem hiding this comment.
force is a required boolean argument and has no default. Transfers to EOAs or non-LSP1 contracts are rejected only when the caller explicitly passes force=false; force=true permits them. Please replace "by default" here and in the other LSP7/LSP8 comparisons.
LUKSOAgent
left a comment
LUKSOAgent
left a comment
There was a problem hiding this comment.
Final pass on ce77bd56: most earlier findings are fixed, including recovery permissions, EIP-7702 persistence and sponsorship, LSP8 metadata semantics, the LSP7 revokeOperator ABI, and per-asset ERC-1155 mapping. Five points remain in the inline comments. Local dependency install, Prettier, the Docusaurus production build, and git diff --check all pass. The new head's GitHub workflows are still awaiting approval.
| Granting an app a session controller is one transaction. Revoking it is one transaction. The Universal Profile checks permissions on every call through [LSP20](../../../standards/accounts/lsp20-call-verification.md), so the scope is enforced on-chain, not at a wallet UI layer that a malicious or compromised frontend could bypass. Core LSP6 has no controller-expiry field — a controller's permissions stay live until someone explicitly revokes them in a transaction. An app can layer its own off-chain "session" convention on top (e.g. stop using a key after N hours), but that's an app-level policy, not something the account itself enforces; a direct call with that controller's key still works until the on-chain permission is actually revoked. | ||
|
|
||
| :::tip Ask for less, by default | ||
| A Universal Profile lets a dApp request exactly the permission it needs — one token, one function, one time window — instead of defaulting to full account access just because there was no standard way to ask for less. |
There was a problem hiding this comment.
Line 40 now correctly explains that core LSP6 has no controller expiry, but this tip still advertises one time window as part of the permission scope. LSP25 validity timestamps limit one relay signature, not the controller's continuing authority. Please remove this phrase or explicitly scope it to a single LSP25 relay.
|
|
||
| - **Atomic pause + burn** — if your contract does have a pause/freeze modifier (common on OpenZeppelin-based deployments that added `Pausable`), pause it in the same transaction or block as the final snapshot, so nothing can transfer after the snapshot but before the new contracts go live. | ||
| - **Escrow-and-claim** — holders deposit their ERC1155 balance into an escrow or burn contract in exchange for the new LSP7/LSP8 tokens. This works even without a pause function, since it only relies on the standard `safeTransferFrom`, not owner-level control. | ||
| - **Replay-protected snapshot with a reconciliation window** — snapshot, then delay activating the new contracts' claim/mint path long enough to catch and reconcile any transfers that landed after the snapshot, with a documented policy for resolving disputes. |
There was a problem hiding this comment.
A reconciliation window by itself is not replay protection. Delaying activation and reconciling interim transfers does not stop the legacy ERC-1155 balances from moving again after the new claims go live, so both representations can still be spent. Please remove this option or require a persistent on-chain cutoff or consume mechanism, such as claim-time escrow or burn, then also remove past the reconciliation window from the verification item.
| | Criterion | Ethereum L1 | Base | Polygon | Solana | LUKSO | | ||
| | ------------------- | ------------------------------------------------------------------------------------------------------- | ------------------------ | ------------------------ | --------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | Secondary liquidity | deepest | fastest-growing EVM | strong enterprise mints | deepest non-EVM | early | | ||
| | Royalty enforcement | [ERC-2981](https://eips.ethereum.org/EIPS/eip-2981) signal | signal | signal | enforced on compressed NFTs | **LSP18** — the named LUKSO royalty standard, but not yet fleshed out on-chain (no royalty events defined in the current spec); LSP4 itself carries no royalty field | |
There was a problem hiding this comment.
No royalty events does not mean LSP18 is not fleshed out on-chain. The current LSP-18 RFC already defines ERC725Y data keys for royalty recipients, percentages, and the enforcement hint; it does not itself enforce marketplace payment. Please describe it as an RFC metadata signal with marketplace-discretionary enforcement, and apply the same correction to creator-platforms.md.
| | Transfer context payload | ❌ none — bolted on via wrapper contracts | ✅ native `bytes data` on every transfer | | ||
| | Authorization | `approve` / `allowance` / `transferFrom` | `authorizeOperator` — scoped, revocable, and notifies the operator | | ||
| | Metadata | `name()` / `symbol()` / `decimals()` only | ✅ unlimited [ERC725Y](../../../standards/erc725.md) key-value storage under [LSP4](../../../standards/tokens/LSP4-Digital-Asset-Metadata.md) | | ||
| | Accidental-transfer protection | ❌ none | ✅ required `force` flag — callers must explicitly pass `force=false` to reject transfers to non-receiving contracts | |
There was a problem hiding this comment.
force is now correctly shown as required, but the false branch is still incomplete: force=false requires the recipient to be an LSP1 contract, so it rejects EOAs as well as non-LSP1 contracts; force=true permits either. Please state both cases here and mirror the fix in erc721-vs-lsp8.md and erc20-transfer-hooks.md.
| runs-on: ubuntu-latest | ||
| needs: checks | ||
| permissions: | ||
| pull-requests: write |
There was a problem hiding this comment.
For this public-fork pull_request, GitHub downgrades requested write access to read-only. The previous run showed PullRequests: read and empty Cloudflare secrets, so this block does not fix the current comment or deploy path. Please revert the ineffective permission change or move only the comment into a separately secured trusted or proxy workflow that never executes fork code. If this block remains for same-repository PRs, also include contents: read, since unspecified permissions become none.
LUKSOAgent
left a comment
LUKSOAgent
left a comment
There was a problem hiding this comment.
Re-reviewed the current head. All five remaining technical comments are correctly addressed. Approved.
|
Thanks a lot for this @tehnalogos! 🙏🏼 Merged! |
3 participants
Uh oh!
There was an error while loading. Please reload this page.