feat: implementation spike#4
Open
stevieraykatz wants to merge 1 commit into
Open
Conversation
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Notes about current state:
IPolicyRegistry has no redeemer slot (BLOCKER for redeem's promised semantics). IDefaultToken.redeem NatSpec says policy check "consults the policy for msg.sender via the redeemer slot of a compound policy" and that "tokens without redemption configure that slot as always-reject". But IPolicyRegistry.CompoundPolicyData only has senderPolicyId / recipientPolicyId / mintRecipientPolicyId — no redeemer slot, and no isAuthorizedRedeemer(...) view. As written there is no way to wire the promised behavior. I'm temporarily mapping redeem's policy check onto isAuthorizedSender(transferPolicyId, msg.sender) with a TODO in the code; the registry interface needs a fourth slot before this is real. Worth a note to Amie because it directly affects the "tokens without redemption" pattern she's documenting in the NatSpec.
EIP-712 domain wallet-compat risk. Interface says domain content is chainId + verifyingContract only with name/version empty, and eip712Domain().fields == 0x0c. I implemented the ERC-5267-pedantic version: the EIP712Domain TYPEHASH itself drops name and version, so the canonical OZ permit signing path that uses the full 5-field type hash will produce signatures that don't verify. If the intent was "still use the canonical 5-field type hash, just leave name/version empty" (OZ-style, MetaMask-compatible, fields-bitfield slightly misleading), the TYPEHASH and the domain separator computation should change. This is a one-character question to Amie that determines whether real wallets can sign permits.
Constructor / factory shape isn't specified yet. I matched ITokenFactory.CreateDefaultTokenParams positionally (less salt), bootstrap-minting initialSupply to initialSupplyRecipient and bypassing the policy check per that NatSpec. If/when ITokenFactory ships as a concrete contract, this needs revisiting — likely take the struct directly.
Policy validation in changeTransferPolicyId for built-in IDs is silent. I trap IDs 0 and 1 as always-valid in B20 without calling the registry. The NatSpec says "MUST exist in the registry (or be one of the built-in IDs 0 or 1)". That phrasing is satisfied, but worth confirming Amie doesn't want a registry call for the built-ins too (would catch a configuration where the registry is somehow wrong).
Bootstrap mint emits Transfer from address(0) but no MINT_ROLE check / no MINT pause / no policy check. That matches CreateDefaultTokenParams.initialSupply NatSpec on the factory side ("bypasses the transfer-policy check"). Worth Amie confirming we also intend to bypass the MINT pause vector and MINT_ROLE for the bootstrap path — those checks would be nonsensical at construction time, but the interface doesn't explicitly say so.
_staticcallPolicyBool safe-by-default behavior is mine, not the spec's. When the registry precompile isn't deployed (or staticcall fails), any non-built-in policy ID denies. The interface doesn't specify what happens when the registry is missing — I chose deny because allow would be a security regression. Worth flagging to Amie that this implementation choice exists.
_POLICY_REGISTRY address is a placeholder (0x42). Marked TODO in code. Need the real precompile address before anything ships.
Renounce-last-admin tracking added an admin counter that's not in the interface. Minor — this is purely an implementation detail to support LastAdminCannotRenounce. No interface impact.