Ownable refactor -> replace pk with sk#466
Ownable refactor -> replace pk with sk#466andrew-fleming wants to merge 11 commits intoOpenZeppelin:mainfrom
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR migrates the Ownable access control module from pubkey-based ownership ( Changes
Sequence Diagram(s)sequenceDiagram
actor Test as Test Suite
participant Contract as Ownable Contract
participant Witness as wit_OwnableSK Witness
participant Hash as persistentHash
Test->>Contract: initialize(Either::Left(accountId))
Test->>Contract: assertOnlyOwner() / transferOwnership()
Note over Contract: Authorization check needed
Contract->>Witness: wit_OwnableSK()
Witness-->>Contract: [privateState, secretKey]
Contract->>Hash: persistentHash(secretKey)
Hash-->>Contract: computed accountId (Bytes<32>)
Contract->>Contract: compare computed ≟ _owner.left
alt Owner matches
Contract-->>Test: ✓ Operation allowed
else Owner mismatch
Contract-->>Test: ✗ Revert
end
sequenceDiagram
actor Caller as Caller
participant SimCtx as OwnableSimulator
participant PrivState as privateState
participant ContractCall as Contract Execution
Caller->>SimCtx: Create simulator instance
Caller->>PrivState: injectSecretKey(newSK)
PrivState-->>Caller: Updated state
Caller->>SimCtx: transferOwnership(newOwner)
SimCtx->>SimCtx: computeAccountId(newSK)
SimCtx->>ContractCall: Call contract with witness context
ContractCall-->>SimCtx: Execute with injected secret key
SimCtx-->>Caller: Result + validated state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
andrew-fleming
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/src/access/test/simulators/OwnableSimulator.ts`:
- Around line 125-149: injectSecretKey currently stores the provided Uint8Array
verbatim which breaks the 32-byte invariant and allows external mutation; change
injectSecretKey to call OwnablePrivateState.withSecretKey(newSK) (which enforces
length and makes a defensive copy) and pass that returned state into
circuitContextManager.updatePrivateState; also ensure getCurrentSecretKey
continues to return the stored key but consider returning a copy if
withSecretKey does not already guarantee immutability (refer to
OwnablePrivateState.withSecretKey and OwnableWitnesses.ts for the expected
behavior).
In `@contracts/src/access/witnesses/OwnableWitnesses.ts`:
- Around line 5-18: The file imports a concrete Ledger type from artifacts which
couples this helper to MockOwnable; remove the Ledger import and make the
WitnessContext ledger type generic/agnostic (e.g., use WitnessContext<unknown,
P> or WitnessContext<any, P>) in the IOwnableWitnesses.wit_OwnableSK signature
so the module only depends on the private state type P; keep the WitnessContext
type import but stop referencing MockOwnable Ledger here and instead bind the
concrete ledger type in simulator/tests where the interface is implemented.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 70e1d33e-6c3b-49a4-9e4b-fcc0f2aa3c80
📒 Files selected for processing (6)
contracts/src/access/Ownable.compactcontracts/src/access/test/Ownable.test.tscontracts/src/access/test/mocks/MockOwnable.compactcontracts/src/access/test/simulators/OwnableSimulator.tscontracts/src/access/witnesses/OwnableWitnesses.tscontracts/src/access/witnesses/test/OwnableWitnesses.test.ts
0xisk
left a comment
0xisk
left a comment
There was a problem hiding this comment.
Looking good! Thank you @andrew-fleming!
| it('should allow transfers to zero', () => { | ||
| ownable._transferOwnership(utils.ZERO_KEY); | ||
| expect(ownable.owner()).toEqual(utils.ZERO_KEY); | ||
| ownable._transferOwnership(ZERO_ACCOUNT); | ||
| expect(ownable.owner()).toEqual(ZERO_ACCOUNT); |
There was a problem hiding this comment.
🔵 followup: I think that's a repeated test ` it('should allow transfers to zero', () => {` on line 427
| * @param {Either<Bytes<32>, ContractAddress>} target - The value to check. | ||
| * @returns {Boolean} - `true` if the active branch is zero, `false` otherwise. | ||
| */ | ||
| circuit _isTargetZero(target: Either<Bytes<32>, ContractAddress>): Boolean { |
There was a problem hiding this comment.
🔵 followup: That should be generalized in Utils.compact
There was a problem hiding this comment.
🔵 followup: OR we generalize the left side of the isKeyOrAddressZero.
There was a problem hiding this comment.
I think the former. The latter's name doesn't work and I don't (currently) see any case where the first type would not be Bytes<32>
| * | ||
| * @returns {Bytes<32>} accountId - The computed account identifier. | ||
| */ | ||
| export pure circuit computeAccountId(secretKey: Bytes<32>): Bytes<32> { |
There was a problem hiding this comment.
🔵 followup: I see the pattern of using those circuits, was thinking if we have a module that only has AccountId state and it's circuit. Then it is reused by all the other modules.
| const isContractAddr = !newOwner.is_left; | ||
| assert(!isContractAddr, "Ownable: unsafe ownership transfer"); |
There was a problem hiding this comment.
🔵 followup: as discussed on the AC PR, a generalied Left for the isContractAddress would be better.
| const isContractAddr = !newOwner.is_left; | ||
| assert(!isContractAddr, "Ownable: unsafe ownership transfer"); |
There was a problem hiding this comment.
🔵 followup: Related to a generalized isContractAdd
| /** | ||
| * @description Transfers ownership of the contract to `newOwner`. | ||
| * | ||
| * @circuitInfo k=10, rows=338 |
| * Useful for deterministic nonce generation or advanced use cases. | ||
| * | ||
| * @param sk - The 32-byte secret nonce to use. | ||
| * @returns A fresh OwnablePrivateState instance with the provided nonce. |
There was a problem hiding this comment.
⚪ nitpick: I see the same here of using nonce although the contract says its key I think it is better to unify the terms.
There was a problem hiding this comment.
🔴 blocking: let's get back the removed @circuitInfo` s.
Co-authored-by: 0xisk <0xisk@proton.me> Signed-off-by: Andrew Fleming <fleming.andrew@protonmail.com>
Co-authored-by: 0xisk <0xisk@proton.me> Signed-off-by: Andrew Fleming <fleming.andrew@protonmail.com>
2 participants
Types of changes
What types of changes does your code introduce to OpenZeppelin Midnight Contracts?
Put an
xin the boxes that applyFixes #453
Summary by CodeRabbit
New Features
Updates
Tests