Omnibus Extension #186
Omnibus Extension #186
Conversation
✅ Deploy Preview for confidential-tokens ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Reviewer's GuideImplement ERC7984Omnibus extension by adding two new confidential transfer methods that decrypt external inputs, configure FHE ACL for sender and recipient, delegate to base transfer logic, and emit OmnibusTransfer events. Support is added in mocks with helper methods, complemented by end-to-end tests and a version changeset. Sequence diagram for confidentialTransferFromOmnibus methodsequenceDiagram
participant Caller
participant ERC7984Omnibus
participant FHE
participant ERC7984
Caller->>ERC7984Omnibus: confidentialTransferFromOmnibus(omnibusFrom, omnibusTo, sender, recipient, externalAmount, inputProof)
ERC7984Omnibus->>FHE: fromExternal(recipient, inputProof)
ERC7984Omnibus->>FHE: fromExternal(sender, inputProof)
ERC7984Omnibus->>FHE: allowThis(recipient_)
ERC7984Omnibus->>FHE: allow(recipient_, omnibusTo)
ERC7984Omnibus->>FHE: allow(recipient_, omnibusFrom)
ERC7984Omnibus->>FHE: allowThis(sender_)
ERC7984Omnibus->>FHE: allow(sender_, omnibusTo)
ERC7984Omnibus->>FHE: allow(sender_, omnibusFrom)
ERC7984Omnibus->>ERC7984: confidentialTransferFrom(omnibusFrom, omnibusTo, externalAmount, inputProof)
ERC7984Omnibus-->>Caller: emit OmnibusTransfer(omnibusFrom, omnibusTo, sender_, recipient_, transferred)
ERC7984Omnibus-->>Caller: return transferred
ER diagram for OmnibusTransfer event data structureerDiagram
OMNIBUSTRANSFER {
address omnibusFrom
address omnibusTo
eaddress sender
eaddress recipient
euint64 amount
}
Class diagram for ERC7984Omnibus extension and related contractsclassDiagram
class ERC7984 {
<<abstract>>
+confidentialTransfer()
+confidentialTransferFrom()
}
class ERC7984Omnibus {
<<abstract>>
+confidentialTransferOmnibus(omnibusTo, sender, recipient, externalAmount, inputProof)
+confidentialTransferFromOmnibus(omnibusFrom, omnibusTo, sender, recipient, externalAmount, inputProof)
+OmnibusTransfer event
}
class ERC7984Mock {
+_mint(to, encryptedAmount, inputProof)
+$_mint(to, amount)
+$_transfer(from, to, amount)
}
class ERC7984OmnibusMock {
<<abstract>>
+_update(from, to, amount)
}
ERC7984Omnibus --|> ERC7984
ERC7984OmnibusMock --|> ERC7984Omnibus
ERC7984OmnibusMock --|> ERC7984Mock
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new abstract extension Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Omnibus as ERC7984Omnibus
participant FHE as FHE Lib
participant Core as ERC7984 (core)
participant ACL as ACL
rect rgb(250,250,255)
note right of Caller: External-proof path (single proof contains sender, recipient, amount)
Caller->>Omnibus: confidentialTransferOmnibus(..., externalAmount, inputProof)
Omnibus->>FHE: fromExternal(inputProof)
FHE-->>Omnibus: (sender, recipient, amount)
end
rect rgb(245,255,245)
note right of Caller: Handle-based path (separate encrypted handles)
Caller->>Omnibus: confidentialTransferOmnibus(..., senderHandle, recipientHandle, amountHandle)
Omnibus->>FHE: resolve handles & set allow permissions
FHE-->>Omnibus: (sender, recipient, amount)
end
Omnibus->>FHE: allowThis(sender, recipient, amount)
Omnibus->>FHE: allow(omnibusAddr, amount)
Omnibus->>ACL: (implicit ACL checks)
Omnibus->>Core: confidentialTransfer(sender, recipient, amount)
Core-->>Omnibus: euint64 transferred
Omnibus-->>Caller: euint64 transferred
Omnibus-->>Caller: emit OmnibusConfidentialTransfer(omnibusFrom?, omnibusTo, sender, recipient, transferred)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `test/token/ERC7984/extensions/ERC7984Omnibus.test.ts:34` </location>
<code_context>
+ it('normal transfer', async function () {
</code_context>
<issue_to_address>
Missing test for invalid encrypted input or proof.
Add a test case with invalid or tampered encrypted input or inputProof to confirm the contract rejects these transfers and does not emit OmnibusTransfer.
Suggested implementation:
```typescript
it('normal transfer', async function () {
const caller = transferFrom ? this.operator : this.holder;
const encryptedInput = await fhevm
.createEncryptedInput(this.token.target, caller.address)
.addAddress(this.holder.address)
.addAddress(this.subaccount.address)
.add64(100)
.encrypt();
const args = [
this.recipient.address,
];
// ... rest of normal transfer test
});
it('reverts on tampered encrypted input or proof', async function () {
const caller = transferFrom ? this.operator : this.holder;
// Create a valid encrypted input
const encryptedInput = await fhevm
.createEncryptedInput(this.token.target, caller.address)
.addAddress(this.holder.address)
.addAddress(this.subaccount.address)
.add64(100)
.encrypt();
// Tamper with the encrypted input (e.g., flip a byte)
let tamperedInput = encryptedInput;
if (typeof tamperedInput === 'string') {
tamperedInput = '0x' + tamperedInput.slice(2, -2) + (tamperedInput.slice(-2) === '00' ? '01' : '00');
} else if (Buffer.isBuffer(tamperedInput)) {
tamperedInput = Buffer.from(tamperedInput);
tamperedInput[0] ^= 0xff;
}
const args = [
this.recipient.address,
tamperedInput,
this.inputProof, // assuming inputProof is available in the test context
];
const contractMethod = transferFrom
? this.token.connect(caller).omnibusTransferFrom
: this.token.connect(caller).omnibusTransfer;
await expect(
contractMethod(...args)
).to.be.revertedWith('Invalid encrypted input or proof');
// Optionally, check that OmnibusTransfer event was NOT emitted
// (If using event assertions, e.g. with chai)
});
```
- Ensure that `this.inputProof` is available in the test context. If not, you may need to generate or mock it similarly to how `encryptedInput` is created.
- Adjust the tampering logic if your encrypted input format is different.
- If your contract uses a different revert message, update `'Invalid encrypted input or proof'` to match the actual revert reason.
- If you want to test tampering with `inputProof` instead, add a similar test where you modify `inputProof` instead of `encryptedInput`.
</issue_to_address>
### Comment 2
<location> `test/token/ERC7984/extensions/ERC7984Omnibus.test.ts:76` </location>
<code_context>
+ fhevm.userDecryptEuint(FhevmType.euint64, omnibusTransferEvent.args[4], this.token.target, this.holder),
+ ).to.eventually.equal(100);
+
+ await expect(this.acl.isAllowed(omnibusTransferEvent.args[2], this.holder)).to.eventually.be.true;
+ await expect(this.acl.isAllowed(omnibusTransferEvent.args[2], this.recipient)).to.eventually.be.true;
+ await expect(this.acl.isAllowed(omnibusTransferEvent.args[2], this.operator)).to.eventually.be.false;
+ });
+
</code_context>
<issue_to_address>
Consider adding assertions for balance changes.
Adding assertions for token balances after transfers will help verify that the confidential transfer logic correctly updates all relevant accounts.
Suggested implementation:
```typescript
// Get balances before transfer
const holderBalanceBefore = await this.token.balanceOf(this.holder.address);
const recipientBalanceBefore = await this.token.balanceOf(this.recipient.address);
const subaccountBalanceBefore = await this.token.balanceOf(this.subaccount.address);
await expect(
fhevm.userDecryptEaddress(omnibusTransferEvent.args[2], this.token.target, this.holder),
).to.eventually.equal(this.holder.address);
await expect(
fhevm.userDecryptEaddress(omnibusTransferEvent.args[3], this.token.target, this.holder),
).to.eventually.equal(this.subaccount.address);
await expect(
fhevm.userDecryptEuint(FhevmType.euint64, omnibusTransferEvent.args[4], this.token.target, this.holder),
).to.eventually.equal(100);
// Get balances after transfer
const holderBalanceAfter = await this.token.balanceOf(this.holder.address);
const recipientBalanceAfter = await this.token.balanceOf(this.recipient.address);
const subaccountBalanceAfter = await this.token.balanceOf(this.subaccount.address);
// Assert balance changes
expect(holderBalanceAfter).to.equal(holderBalanceBefore.sub(100));
expect(recipientBalanceAfter).to.equal(recipientBalanceBefore.add(100));
// If subaccount is involved in the transfer, assert its balance as well
// expect(subaccountBalanceAfter).to.equal(subaccountBalanceBefore); // Adjust as needed
```
- If your confidential transfer logic involves the subaccount balance changing, adjust the subaccount assertion accordingly.
- Make sure that `this.token.balanceOf` returns the correct type (BigNumber) for arithmetic operations.
- If the transfer amount is not always 100, replace `100` with the actual transfer amount variable.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
.changeset/stupid-fans-bow.md (1)
5-5: Tighten the changeset description (use extension-focused phrasing).This mirrors an earlier suggestion. Recommend wording that clearly frames this as an ERC7984 extension and calls out omnibus subaccounts.
-`ERC7984Omnibus`: Add an extension of `ERC7984` that exposes new functions for transferring between confidential subaccounts on omnibus wallets. +`ERC7984Omnibus`: Add an `ERC7984` extension exposing omnibus confidential transfers between confidential subaccounts on omnibus wallets: +- `confidentialTransferOmnibus(...)` +- `confidentialTransferFromOmnibus(...)` +Emits `OmnibusTransfer` for omnibus transfer operations.test/token/ERC7984/extensions/ERC7984Omnibus.test.ts (1)
34-79: Add negative test for tampered encrypted input/proof.You already validate the happy path; add a revert test for modified
handlesorinputProofto ensure the contract rejects malformed inputs.I can draft a focused spec that flips a byte in one handle or the proof and asserts a revert (with your actual revert string).
contracts/token/ERC7984/extensions/ERC7984Omnibus.sol (1)
35-37: Bind emitted sub-wallet metadata to authorizations (attestation + replay protection)As-is,
sender_/recipient_used in the event are derived from external inputs but not cryptographically tied toomnibusFrom/omnibusToor operator approval; a malicious operator could misreport sub-wallets in events without affecting token state. If off-chain ledgers rely on these events, add an attested payload (EIP-712) signed by the omnibus manager (and optionally the recipient side) includingsender,recipient,amount,omnibusFrom/To,nonce,deadline.Sketch:
- function confidentialTransferFromOmnibus(..., bytes calldata inputProof) external returns (euint64) { + function confidentialTransferFromOmnibus( + ..., + bytes calldata inputProof, + bytes calldata omnibusFromSig + ) external returns (euint64) { + _verifyOmnibusFromSig(omnibusFrom, sender, recipient, externalAmount, omnibusTo, nonce, deadline, omnibusFromSig); ... emit OmnibusTransfer(omnibusFrom, omnibusTo, sender_, recipient_, transferred);Add nonce management to prevent replay.
Also applies to: 59-61 </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (11)</summary><blockquote> <details> <summary>.changeset/stupid-fans-bow.md (1)</summary><blockquote> `6-6`: **Remove stray character.** The trailing “6” looks accidental; it will leak into release notes. ```diff -6contracts/mocks/token/ERC7984Mock.sol (1)
34-36: Plaintext mint helper looks good; add a brief NatSpec note.The overload correctly wraps
uint64viaFHE.asEuint64. Add a short doc comment to make its testing purpose explicit.- function $_mint(address to, uint64 amount) public returns (euint64 transferred) { + /// @dev Test helper: mints a plaintext `uint64` amount by encoding it as `euint64`. + function $_mint(address to, uint64 amount) public returns (euint64 transferred) { return _mint(to, FHE.asEuint64(amount)); }test/token/ERC7984/extensions/ERC7984Omnibus.test.ts (4)
26-33: Cover unauthorized operator for transferFrom.When
transferFromis true without priorsetOperatoror with insufficient allowance, the call should revert. Add a test to lock this in.beforeEach(async function () { if (transferFrom) { await this.token.connect(this.holder).setOperator(this.operator.address, 999999999999); } }); + + if (transferFrom) { + it('reverts when operator is not authorized or allowance is insufficient', async function () { + const { holder, operator } = this; + // remove or reduce allowance + await this.token.connect(holder).setOperator(operator.address, 0); + await expect( + this.token + .connect(operator) + ['confidentialTransferFromOmnibus(address,address,bytes32,bytes32,bytes32,bytes)']( + holder.address, + this.recipient.address, + ethers.ZeroHash, ethers.ZeroHash, ethers.ZeroHash, '0x' + ) + ).to.be.reverted; + }); + }
81-117: Exercise the “direct amount” variants too.The PR advertises support for “external proofs” and “direct amounts,” but tests only cover the former. Add cases that invoke the direct-amount overloads to ensure both paths emit
OmnibusTransferwith consistent semantics.Point me to the exact direct-amount signatures you settled on, and I’ll provide drop-in tests using them.
60-75: Optional: assert static-call return equals event amount.If the functions return the transferred
euint64, add a.staticCallprior to sending the tx and compare to the event’s encrypted amount handle.- const tx = await this.token + const returned = await this.token .connect(caller) - [ + .staticCall[ transferFrom ? 'confidentialTransferFromOmnibus(address,address,bytes32,bytes32,bytes32,bytes)' : 'confidentialTransferOmnibus(address,bytes32,bytes32,bytes32,bytes)' - ](...args); + ](...args); + const tx = await this.token.connect(caller)[ + transferFrom + ? 'confidentialTransferFromOmnibus(address,address,bytes32,bytes32,bytes32,bytes)' + : 'confidentialTransferOmnibus(address,bytes32,bytes32,bytes32,bytes)' + ](...args); const omnibusTransferEvent = (await tx.wait()).logs.filter( (log: any) => log.fragment?.name === 'OmnibusTransfer', )[0]; + expect(omnibusTransferEvent.args[4]).to.equal(returned);
60-65: Minor: also assert sender/recipient in the “over-balance” test.You only check the amount there; repeat the address assertions for symmetry.
- const omnibusTransferEvent = (await tx.wait()).logs.filter( + const omnibusTransferEvent = (await tx.wait()).logs.filter( (log: any) => log.fragment?.name === 'OmnibusTransfer', )[0]; + expect(omnibusTransferEvent.args[0]).to.equal(this.holder.address); + expect(omnibusTransferEvent.args[1]).to.equal(this.recipient.address);contracts/token/ERC7984/extensions/ERC7984Omnibus.sol (5)
9-15: Event indexing: consider indexing sender instead of recipient + add brief NatSpecConsumers are more likely to filter by the originating sub-wallet. With two indexed L1 addresses already, only one eaddress can be indexed. Consider flipping the indexed field and add a short NatSpec block to document the privacy/observability intent.
event OmnibusTransfer( address indexed omnibusFrom, address indexed omnibusTo, - eaddress sender, - eaddress indexed recipient, + eaddress indexed sender, + eaddress recipient, euint64 amount );
17-23: Use external instead of public for gas and intentFunctions are not called internally;
externalis cheaper and communicates API surface.-function confidentialTransferOmnibus( +function confidentialTransferOmnibus( address omnibusTo, externalEaddress sender, externalEaddress recipient, externalEuint64 externalAmount, bytes calldata inputProof -) public returns (euint64) { +) external returns (euint64) { -function confidentialTransferFromOmnibus( +function confidentialTransferFromOmnibus( address omnibusFrom, address omnibusTo, externalEaddress sender, externalEaddress recipient, externalEuint64 externalAmount, bytes calldata inputProof -) public returns (euint64) { +) external returns (euint64) {Also applies to: 40-47
24-34: Re-evaluateallowThisand duplicate ACL grants
sender_andrecipient_are only emitted, not used by on-chain logic. If the contract never operates on these ciphertexts,FHE.allowThis(...)is likely unnecessary. Also, ACL grants largely duplicate across both functions.- FHE.allowThis(recipient_); FHE.allow(recipient_, omnibusTo); - FHE.allow(recipient_, msg.sender); + FHE.allow(recipient_, msg.sender); // keep if the operator must view - FHE.allowThis(sender_); FHE.allow(sender_, omnibusTo); FHE.allow(sender_, msg.sender);If you need these grants in multiple places, consider a tiny helper:
function _allow(eaddress who, address a, address b) internal { FHE.allow(who, a); FHE.allow(who, b); }Also applies to: 51-56
51-58: Align ACLs between the two entry pointsIn
confidentialTransferFromOmnibus, the operator (msg.sender) isn’t granted access tosender_/recipient_, unlike the non-From variant. If operators need parity for off-chain reconciliation, mirror the grants; if not, add a short comment stating the rationale.FHE.allow(recipient_, omnibusTo); FHE.allow(recipient_, omnibusFrom); + // Optional (parity with confidentialTransferOmnibus): + // FHE.allow(recipient_, msg.sender); FHE.allow(sender_, omnibusTo); FHE.allow(sender_, omnibusFrom); + // Optional (parity with confidentialTransferOmnibus): + // FHE.allow(sender_, msg.sender);
8-16: Add brief NatSpec for contract and eventDocumenting trust assumptions (operator model vs. attested sub-wallet intent), who can decrypt what, and filtering expectations will save downstream integrators time.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
.changeset/stupid-fans-bow.md(1 hunks)contracts/mocks/token/ERC7984Mock.sol(2 hunks)contracts/mocks/token/ERC7984OmnibusMock.sol(1 hunks)contracts/token/ERC7984/extensions/ERC7984Omnibus.sol(1 hunks)test/token/ERC7984/extensions/ERC7984Omnibus.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/token/ERC7984/extensions/ERC7984Omnibus.test.ts (1)
test/helpers/accounts.ts (1)
ACL_ADDRESS(7-7)
🔇 Additional comments (1)
contracts/mocks/token/ERC7984OmnibusMock.sol (1)
10-16: Override chain is correct;super._updatepreservesERC7984Mockbehavior.With inheritance
ERC7984Omnibus, ERC7984Mock,super._updateresolves throughERC7984MocktoERC7984, keeping theFHE.allowside effect from the mock. No change needed.If you want to double-check linearization, run a quick compile locally and ensure tests still observe ACL allowances on the emitted
eaddress.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (12)
test/token/ERC7984/extensions/ERC7984Omnibus.test.ts (6)
15-21: Tighten contract typing to avoidanycasting.Leverage the generated factory for
$ERC7984OmnibusMockto keep strong typings and catch signature drifts at compile time.-const token = (await ethers.deployContract('$ERC7984OmnibusMock', [ - name, - symbol, - uri, -])) as any as $ERC7984OmnibusMock; +const factory = await ethers.getContractFactory('$ERC7984OmnibusMock'); +const token = (await factory.deploy(name, symbol, uri)) as unknown as $ERC7984OmnibusMock; +await token.waitForDeployment();
23-24: Prefer BigInt literals for u64 semantics in tests.Use
1000nto mirror uint64 intent and avoid accidental JS number coercion.-await this.token['$_mint(address,uint64)'](this.holder.address, 1000); +await this.token['$_mint(address,uint64)'](this.holder.address, 1000n);
34-66: Use event assertions instead of manual log filtering.
expect(tx).to.emit(...).withArgs(...)is clearer and fails with better diffs when the event isn’t emitted or args mismatch.-const tx = await this.token +const tx = await this.token .connect(caller) [ transferFrom ? 'confidentialTransferFromOmnibus(address,address,bytes32,bytes32,bytes32,bytes)' : 'confidentialTransferOmnibus(address,bytes32,bytes32,bytes32,bytes)' ](...args); -const omnibusConfidentialTransferEvent = (await tx.wait()).logs.filter( - (log: any) => log.fragment?.name === 'OmnibusConfidentialTransfer', -)[0]; -expect(omnibusConfidentialTransferEvent.args[0]).to.equal(this.holder.address); -expect(omnibusConfidentialTransferEvent.args[1]).to.equal(this.recipient.address); +await expect(tx) + .to.emit(this.token, 'OmnibusConfidentialTransfer') + .withArgs( + transferFrom ? this.holder.address : caller.address, + this.recipient.address, + anyValue, // sender (eaddress) + anyValue, // recipient (eaddress) + anyValue // amount (euint64) + ); + +const receipt = await tx.wait(); +const omnibusConfidentialTransferEvent = receipt.logs.find((l: any) => l.fragment?.name === 'OmnibusConfidentialTransfer');(Add
const { anyValue } = require('@nomicfoundation/hardhat-chai-matchers/withArgs');at top if not imported.)
66-86: Assert ACL on both sender and recipient ciphertexts.Current ACL checks validate permissions on the sender ciphertext only. Consider also asserting ACL on the recipient ciphertext for symmetry.
await expect( fhevm.userDecryptEaddress(omnibusConfidentialTransferEvent.args[2], this.token.target, this.holder), ).to.eventually.equal(this.holder.address); await expect( fhevm.userDecryptEaddress(omnibusConfidentialTransferEvent.args[3], this.token.target, this.holder), ).to.eventually.equal(this.subaccount.address); @@ await expect(this.acl.isAllowed(omnibusConfidentialTransferEvent.args[2], this.operator)).to.eventually.be .false; + +// Mirror ACL checks on the recipient ciphertext +await expect(this.acl.isAllowed(omnibusConfidentialTransferEvent.args[3], this.holder)).to.eventually.be.true; +await expect(this.acl.isAllowed(omnibusConfidentialTransferEvent.args[3], this.recipient)).to.eventually.be.true; +await expect(this.acl.isAllowed(omnibusConfidentialTransferEvent.args[3], this.operator)).to.eventually.be.false;
88-131: Strengthen “over-balance” scenario with pre/post balance assertions.Even if confidential logic clamps to 0, asserting observable balances avoids silent regressions in settlement paths.
-const tx = await this.token +const balanceBefore = await this.token.balanceOf(this.recipient.address); +const tx = await this.token .connect(caller) [ transferFrom ? 'confidentialTransferFromOmnibus(address,address,bytes32,bytes32,bytes32,bytes)' : 'confidentialTransferOmnibus(address,bytes32,bytes32,bytes32,bytes)' ](...args); @@ ).to.eventually.equal(0); + +const balanceAfter = await this.token.balanceOf(this.recipient.address); +expect(balanceAfter).to.equal(balanceBefore);
26-33: Minor: use a finite operator window and assert expiry semantics.Setting an extremely large expiry blurs operator-approval behavior. Consider a short-lived approval and an assertion that calls after expiry revert.
contracts/token/ERC7984/extensions/ERC7984Omnibus.sol (6)
8-14: Document sub-account privacy/visibility guarantees explicitly.Clarify who can decrypt
sender/recipienteaddresses post-transfer and for how long (based onFHE.allow*), since integrators rely on this for compliance.
43-56: Factor out repeated FHE allow rules.The same “permit sender/recipient to A and B” pattern appears across methods. Extract helpers to reduce duplication and risk of divergence.
+ function _allowPair(eaddress a, eaddress b, address x, address y) internal { + FHE.allowThis(a); + FHE.allow(a, x); + FHE.allow(a, y); + FHE.allowThis(b); + FHE.allow(b, x); + FHE.allow(b, y); + } @@ - FHE.allowThis(recipient); - FHE.allow(recipient, omnibusTo); - FHE.allow(recipient, msg.sender); - - FHE.allowThis(sender); - FHE.allow(sender, omnibusTo); - FHE.allow(sender, msg.sender); + _allowPair(recipient, sender, omnibusTo, msg.sender);(Apply similarly in other methods with the appropriate address pairings.)
77-95: Privacy: avoid accidentally granting operator visibility.In the
Fromvariants you intentionally omitmsg.senderfromFHE.allow. Good. Consider a brief comment to codify this invariant and prevent future regressions.function confidentialTransferFromOmnibus( @@ - FHE.allowThis(sender); + // NOTE: Do not grant msg.sender (operator) visibility to preserve holder privacy. + FHE.allowThis(sender);
97-130: Consistency: parameter extraction order.Extraction order of
sender/recipientswaps across methods. Not a bug, but aligning order aids readability.- eaddress recipient = FHE.fromExternal(externalRecipient, inputProof); - eaddress sender = FHE.fromExternal(externalSender, inputProof); + eaddress sender = FHE.fromExternal(externalSender, inputProof); + eaddress recipient = FHE.fromExternal(externalRecipient, inputProof);(Apply similarly to
confidentialTransferFromAndCallOmnibus.)
17-20: Event doc: mentionConfidentialTransfer(AndCall)pairing.The
AndCallvariants still emitOmnibusConfidentialTransfer. Note in the doc that settlement is paired with eitherConfidentialTransferorConfidentialTransferAndCall.
112-130: Gas: preferexternalfor wrapper functions.Wrappers don’t need Solidity’s ABI re-encoding; marking them
externalcan shave gas for calls from EOA/contracts.- ) public virtual returns (euint64) { + ) external virtual returns (euint64) {(Apply to the four wrapper entrypoints that take external proofs.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/token/ERC7984/extensions/ERC7984Omnibus.sol(1 hunks)test/token/ERC7984/extensions/ERC7984Omnibus.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/token/ERC7984/extensions/ERC7984Omnibus.test.ts (1)
test/helpers/accounts.ts (1)
ACL_ADDRESS(7-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: boostsecurity - boostsecurityio/semgrep-pro
- GitHub Check: slither
- GitHub Check: tests
- GitHub Check: coverage
🔇 Additional comments (2)
contracts/token/ERC7984/extensions/ERC7984Omnibus.sol (2)
21-27: Confirm feasibility ofindexed eaddressacross toolchains.Indexing a custom FHE value type in events can be toolchain-sensitive. If
eaddressis a user-defined value type it’s fine; if it’s a struct, topics are unsupported.If needed, drop
indexedonrecipientto maximize compatibility.- eaddress indexed recipient, + eaddress recipient,
57-60: Event semantics: confirmomnibusFrom = msg.senderis intended.For the non-
Fromvariant,omnibusFromismsg.sender. If the omnibus account differs from the caller (meta-tx or forwarding), consider aomnibusFromparameter ormsgSender()abstraction.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
test/token/ERC7984/extensions/ERC7984Omnibus.test.ts (2)
143-147: Same issue: subaccount must be an address.- tx = await this.token.connect(caller).createEncryptedAddress(this.subaccount); + tx = await this.token.connect(caller).createEncryptedAddress(this.subaccount.address);
191-196: Same ACL address-vs-signer issue in the over-balance test.- await expect( - this.acl.isAllowed(omnibusConfidentialTransferEvent.args[2], this.holder), - ).to.eventually.be.true; - await expect( - this.acl.isAllowed(omnibusConfidentialTransferEvent.args[2], this.recipient), - ).to.eventually.be.true; + await expect( + this.acl.isAllowed(omnibusConfidentialTransferEvent.args[2], this.holder.address), + ).to.eventually.be.true; + await expect( + this.acl.isAllowed(omnibusConfidentialTransferEvent.args[2], this.recipient.address), + ).to.eventually.be.true;
🧹 Nitpick comments (6)
test/token/ERC7984/extensions/ERC7984Omnibus.test.ts (6)
100-115: Avoid relying on chai-as-promised ‘eventually’ in decryption checks.If chai-as-promised isn’t globally configured, these will fail. Prefer awaiting the promise explicitly.
- await expect( - fhevm.userDecryptEaddress(omnibusConfidentialTransferEvent.args[2], this.token.target, this.holder), - ).to.eventually.equal(this.holder.address); + expect( + await fhevm.userDecryptEaddress(omnibusConfidentialTransferEvent.args[2], this.token.target, this.holder), + ).to.equal(this.holder.address);(Apply similarly to lines 103–106 and 107–115.)
51-68: DRY the handle-extraction boilerplate and use event assertions.The log filtering +
[0]indexing is brittle and duplicated. Extract a helper to return the first matching arg and useto.emitfor clearer assertions.async function firstEventArg(tx: any, event: string, index: number) { const rc = await tx.wait(); const log = rc.logs.find((l: any) => l.fragment?.name === event); expect(log, `Missing ${event}`).to.not.be.undefined; return log.args[index]; }Then:
- encryptedInput.handles.push( - (await tx.wait()).logs.filter((log: any) => log.fragment?.name === 'EncryptedAddressCreated')[0].args[0], - ); + encryptedInput.handles.push(await firstEventArg(tx, 'EncryptedAddressCreated', 0));Also applies to: 137-154
124-171: Strengthen over-balance assertions.Also assert event presence and decrypted sender/recipient eaddresses, matching the normal-transfer test, to prove consistent semantics when amount clamps to 0.
Also applies to: 172-197
33-35: Magic number for operator allowance/expiry.Extract
const OP_LIMIT = 999_999_999_999n;(or an explicit Max/expiry semantic) for clarity and reuse.
15-21: Prefer typed factories overas anycasts when deploying the mock.Use the generated TypeChain factory for
$ERC7984OmnibusMockto get typed instances and ABI-safe calls.-import { $ERC7984OmnibusMock } from '../../../../types/contracts-exposed/mocks/token/ERC7984OmnibusMock.sol/$ERC7984OmnibusMock'; +import { $ERC7984OmnibusMock, $ERC7984OmnibusMock__factory } from '../../../../types/contracts-exposed/mocks/token/ERC7984OmnibusMock.sol'; ... - const token = (await ethers.deployContract('$ERC7984OmnibusMock', [ - name, - symbol, - uri, - ])) as any as $ERC7984OmnibusMock; + const token = await new $ERC7984OmnibusMock__factory(holder).deploy(name, symbol, uri);
206-230: Avoid brittle string-based function signatures in tests
Tests inERC7984Omnibus.test.tsuse raw signature strings (e.g.'confidentialTransferFromAndCallOmnibus(address,address,bytes32,bytes32,bytes32,bytes,bytes)'), which will silently break if the ABI changes. Use typed contract calls ortoken.interface.encodeFunctionData(“confidentialTransferFromAndCallOmnibus”, […args])to reference ABI fragments by name instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
contracts/mocks/token/ERC7984FreezableMock.sol(0 hunks)contracts/mocks/token/ERC7984Mock.sol(3 hunks)contracts/token/ERC7984/extensions/ERC7984Omnibus.sol(1 hunks)test/token/ERC7984/extensions/ERC7984Omnibus.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- contracts/mocks/token/ERC7984FreezableMock.sol
🚧 Files skipped from review as they are similar to previous changes (2)
- contracts/token/ERC7984/extensions/ERC7984Omnibus.sol
- contracts/mocks/token/ERC7984Mock.sol
🧰 Additional context used
🧬 Code graph analysis (1)
test/token/ERC7984/extensions/ERC7984Omnibus.test.ts (1)
test/helpers/accounts.ts (1)
ACL_ADDRESS(7-7)
🔇 Additional comments (1)
test/token/ERC7984/extensions/ERC7984Omnibus.test.ts (1)
94-99: Tests correctly referenceOmnibusConfidentialTransfer
TheERC7984Omnibus.solcontract declares and emitsOmnibusConfidentialTransfer, and the tests filter on that exact name—there is noOmnibusTransferevent. Disregard the mismatch concern.Likely an incorrect or invalid review comment.
| tx = await this.token.connect(caller).createEncryptedAddress(this.subaccount); | ||
| encryptedInput.handles.push( | ||
| (await tx.wait()).logs.filter((log: any) => log.fragment?.name === 'EncryptedAddressCreated')[0] | ||
| .args[0], | ||
| ); |
There was a problem hiding this comment.
Pass an address, not a Signer, to createEncryptedAddress.
this.subaccount is a Signer; the contract expects address. This will throw at runtime.
- tx = await this.token.connect(caller).createEncryptedAddress(this.subaccount);
+ tx = await this.token.connect(caller).createEncryptedAddress(this.subaccount.address);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tx = await this.token.connect(caller).createEncryptedAddress(this.subaccount); | |
| encryptedInput.handles.push( | |
| (await tx.wait()).logs.filter((log: any) => log.fragment?.name === 'EncryptedAddressCreated')[0] | |
| .args[0], | |
| ); | |
| tx = await this.token.connect(caller).createEncryptedAddress(this.subaccount.address); | |
| encryptedInput.handles.push( | |
| (await tx.wait()).logs.filter((log: any) => log.fragment?.name === 'EncryptedAddressCreated')[0] | |
| .args[0], | |
| ); |
🤖 Prompt for AI Agents
In test/token/ERC7984/extensions/ERC7984Omnibus.test.ts around lines 57 to 61,
the call passes this.subaccount (a Signer) to createEncryptedAddress which
expects an address; replace the Signer with its address (e.g. await
this.subaccount.getAddress()) when calling createEncryptedAddress so the
contract receives a string address, then use the returned event log extraction
as before.
| await expect( | ||
| this.acl.isAllowed(omnibusConfidentialTransferEvent.args[2], this.holder), | ||
| ).to.eventually.be.true; | ||
| await expect( | ||
| this.acl.isAllowed(omnibusConfidentialTransferEvent.args[2], this.recipient), | ||
| ).to.eventually.be.true; |
There was a problem hiding this comment.
ACL isAllowed expects an address; a Signer is being passed.
Pass .address to avoid ABI encoding errors.
- await expect(
- this.acl.isAllowed(omnibusConfidentialTransferEvent.args[2], this.holder),
- ).to.eventually.be.true;
- await expect(
- this.acl.isAllowed(omnibusConfidentialTransferEvent.args[2], this.recipient),
- ).to.eventually.be.true;
+ await expect(
+ this.acl.isAllowed(omnibusConfidentialTransferEvent.args[2], this.holder.address),
+ ).to.eventually.be.true;
+ await expect(
+ this.acl.isAllowed(omnibusConfidentialTransferEvent.args[2], this.recipient.address),
+ ).to.eventually.be.true;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await expect( | |
| this.acl.isAllowed(omnibusConfidentialTransferEvent.args[2], this.holder), | |
| ).to.eventually.be.true; | |
| await expect( | |
| this.acl.isAllowed(omnibusConfidentialTransferEvent.args[2], this.recipient), | |
| ).to.eventually.be.true; | |
| await expect( | |
| this.acl.isAllowed(omnibusConfidentialTransferEvent.args[2], this.holder.address), | |
| ).to.eventually.be.true; | |
| await expect( | |
| this.acl.isAllowed(omnibusConfidentialTransferEvent.args[2], this.recipient.address), | |
| ).to.eventually.be.true; |
🤖 Prompt for AI Agents
In test/token/ERC7984/extensions/ERC7984Omnibus.test.ts around lines 116 to 121,
the ACL.isAllowed call is being passed a Signer (this.holder / this.recipient)
but the function expects an address string; change the calls to pass
this.holder.address and this.recipient.address respectively so the ABI encoding
receives addresses instead of Signer objects.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/utils/HandleAccessManager.test.ts (1)
15-19: Replace magic number with a named constant; consider loosening the exact error-text match.101 looks arbitrary. Name it to document intent and reduce future churn. Optionally assert with a regex/substring to avoid brittle failures if the message wording changes slightly.
- const handle = await createHandle(this.mock, 101); + const handle = await createHandle(this.mock, UNAUTHORIZED_HANDLE_AMOUNT); - await expect(fhevm.userDecryptEuint(FhevmType.euint64, handle, this.mock.target, this.holder)).to.be.rejectedWith( - `User ${this.holder.address} is not authorized to user decrypt handle ${handle}`, - ); + await expect( + fhevm.userDecryptEuint(FhevmType.euint64, handle, this.mock.target, this.holder), + ).to.be.rejectedWith(new RegExp(`not authorized to user decrypt handle ${handle}`));Add once near the top (inside the describe or file scope):
const UNAUTHORIZED_HANDLE_AMOUNT = 101;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/utils/HandleAccessManager.test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: boostsecurity - boostsecurityio/semgrep-pro
- GitHub Check: slither
- GitHub Check: coverage
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contracts/token/ERC7984/extensions/ERC7984Omnibus.sol(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: boostsecurity - boostsecurityio/semgrep-pro
- GitHub Check: coverage
- GitHub Check: lint
- GitHub Check: tests
- GitHub Check: slither
🔇 Additional comments (10)
contracts/token/ERC7984/extensions/ERC7984Omnibus.sol (10)
8-14: LGTM! Clear documentation with important disclaimerThe contract documentation clearly explains the purpose and includes the crucial note about external sub-account balance tracking.
23-29: LGTM! Event design follows past discussion decisionsThe event structure with
omnibusFrom,omnibusTo, and indexedrecipientfollows the previous discussion about indexing priority and encrypted address limitations.
32-36: LGTM! Clear custom error with helpful guidanceThe custom error provides clear messaging and actionable guidance for developers encountering access control issues.
38-50: LGTM! External proof handling implemented correctlyThe function correctly extracts encrypted values from external proofs and delegates to the internal implementation.
52-59: LGTM! Clean delegation patternThe function correctly delegates to the
confidentialTransferFromOmnibusimplementation usingmsg.senderas the omnibus source.
61-74: LGTM! Consistent external proof handlingThe function follows the same pattern as the transfer variant, correctly handling external proofs and delegating to the internal implementation.
76-97: LGTM! Core omnibus logic with proper access controlThe implementation correctly:
- Validates caller access to encrypted addresses
- Sets up ACL permissions for all involved parties
- Delegates to the base transfer function
- Emits the omnibus event
The ACL setup ensures both omnibus accounts have access to the encrypted addresses and amounts.
99-112: LGTM! Consistent pattern for transfer and call variantThe function follows the established pattern for handling external proofs and delegating to the internal
andCallimplementation.
114-122: LGTM! Clean delegation for transfer and callThe function correctly delegates to the
confidentialTransferFromAndCallOmnibusimplementation.
124-138: LGTM! External proof handling for transfer and callThe function maintains consistency with other external proof variants, correctly extracting values and delegating.
|
@james-toussaint Replying to #186 (comment) The reason we have different allowances is since we need to ensure that there are persistent allowances for event parameters. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
contracts/token/ERC7984/extensions/ERC7984Omnibus.sol (1)
23-29: Event shape looks good given topic limits.Indexing
omnibusFrom,omnibusTo, andrecipientbalances searchability vs. topic cap; non-indexed encryptedsenderis acceptable.
🧹 Nitpick comments (1)
contracts/token/ERC7984/extensions/ERC7984Omnibus.sol (1)
183-199: Document why ACL grants are after the callback in this path.Add a brief note to prevent future reordering “optimizations.”
) internal virtual returns (euint64) { - euint64 transferred = confidentialTransferFromAndCall(omnibusFrom, omnibusTo, amount, data); + // NOTE: In the *AndCall* omnibus path, allowances for sender/recipient/transferred + // are granted AFTER the callback by design (see PR #186 discussion). The callback + // does not consume encrypted addresses; these grants are for event emission and + // future access. + euint64 transferred = confidentialTransferFromAndCall(omnibusFrom, omnibusTo, amount, data);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contracts/token/ERC7984/extensions/ERC7984Omnibus.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-15T14:43:25.623Z
Learnt from: arr00
PR: OpenZeppelin/openzeppelin-confidential-contracts#186
File: contracts/token/ERC7984/extensions/ERC7984Omnibus.sol:140-167
Timestamp: 2025-09-15T14:43:25.623Z
Learning: In ERC7984Omnibus callback functions like confidentialTransferFromAndCallOmnibus, the encrypted sender and recipient addresses are not passed to the callback recipient - only the standard transfer parameters (omnibusFrom, omnibusTo, amount, data) are passed. The ACL grants for the encrypted addresses are for omnibus event emission and future access, not for callback usage.
Applied to files:
contracts/token/ERC7984/extensions/ERC7984Omnibus.sol
🔇 Additional comments (1)
contracts/token/ERC7984/extensions/ERC7984Omnibus.sol (1)
153-172: No change needed — base confidentialTransferFrom already persists ACL fortransferred.The internal _transfer called by confidentialTransferFrom sets FHE.allow(transferred, from), FHE.allow(transferred, to) and FHE.allowThis(transferred); the non-callback Omnibus path therefore doesn't need to re-grant. The callback path's explicit grants for
transferredare redundant.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
contracts/token/ERC7984/extensions/ERC7984Omnibus.sol (2)
97-116: On checking recipient/amount allowances in public wrappers.You intentionally rely on base functions for allowance checks; leaving it here avoids duplication but might look inconsistent to readers.
If you keep this approach, consider a short note in NatSpec for the public wrappers stating “Underlying ERC7984 methods enforce encrypted-handle allowances.”
Also applies to: 118-126, 129-144, 146-159
91-95: Fix: custom errors cannot be used as require messages (compile-time error).Use explicit revert with the custom error.
- require(FHE.isAllowed(sender, msg.sender), ERC7984UnauthorizedUseOfEncryptedAddress(sender, msg.sender)); - require(FHE.isAllowed(recipient, msg.sender), ERC7984UnauthorizedUseOfEncryptedAddress(recipient, msg.sender)); + if (!FHE.isAllowed(sender, msg.sender)) { + revert ERC7984UnauthorizedUseOfEncryptedAddress(sender, msg.sender); + } + if (!FHE.isAllowed(recipient, msg.sender)) { + revert ERC7984UnauthorizedUseOfEncryptedAddress(recipient, msg.sender); + }- require(FHE.isAllowed(sender, msg.sender), ERC7984UnauthorizedUseOfEncryptedAddress(sender, msg.sender)); - require(FHE.isAllowed(recipient, msg.sender), ERC7984UnauthorizedUseOfEncryptedAddress(recipient, msg.sender)); + if (!FHE.isAllowed(sender, msg.sender)) { + revert ERC7984UnauthorizedUseOfEncryptedAddress(sender, msg.sender); + } + if (!FHE.isAllowed(recipient, msg.sender)) { + revert ERC7984UnauthorizedUseOfEncryptedAddress(recipient, msg.sender); + }Also applies to: 155-159
🧹 Nitpick comments (3)
contracts/token/ERC7984/extensions/ERC7984Omnibus.sol (3)
118-126: Doc fix: wrong function reference in dev comment.This overload wraps the non-proof variant; it should reference euint64-only version (single bytes arg).
- /// @dev Wraps the {confidentialTransferAndCall-address-externalEuint64-bytes-bytes} function and emits the {OmnibusConfidentialTransfer} event. + /// @dev Wraps the {confidentialTransferAndCall-address-euint64-bytes} function and emits the {OmnibusConfidentialTransfer} event.
182-190: Clarify callback semantics in docs (no encrypted addresses passed to receiver).Add a brief NOTE to avoid future reordering of ACL grants based on incorrect assumptions.
- /// @dev Handles the ACL allowances, does the transfer with a callback, and emits {OmnibusConfidentialTransfer}. + /// @dev Handles the ACL allowances, does the transfer with a callback, and emits {OmnibusConfidentialTransfer}. + /// NOTE: The callback recipient does not receive encrypted sender/recipient addresses; only (omnibusFrom, omnibusTo, amount, data) are used in the callback. + /// ACL grants for `sender`/`recipient` are for event decryption and future access, not for the callback execution.
169-176: De-duplicate ACL grant boilerplate for gas/readability.Extract helpers to grant address ACLs (and a separate one for
transferred) and call them from both paths.- FHE.allowThis(sender); - FHE.allow(sender, omnibusFrom); - FHE.allow(sender, omnibusTo); - - FHE.allowThis(recipient); - FHE.allow(recipient, omnibusFrom); - FHE.allow(recipient, omnibusTo); + _grantOmnibusAddressACL(sender, omnibusFrom, omnibusTo); + _grantOmnibusAddressACL(recipient, omnibusFrom, omnibusTo);- FHE.allowThis(sender); - FHE.allow(sender, omnibusFrom); - FHE.allow(sender, omnibusTo); - - FHE.allowThis(recipient); - FHE.allow(recipient, omnibusFrom); - FHE.allow(recipient, omnibusTo); + _grantOmnibusAddressACL(sender, omnibusFrom, omnibusTo); + _grantOmnibusAddressACL(recipient, omnibusFrom, omnibusTo);Add these helpers (place anywhere in the contract body):
function _grantOmnibusAddressACL(eaddress who, address omnibusFrom, address omnibusTo) internal { FHE.allowThis(who); FHE.allow(who, omnibusFrom); FHE.allow(who, omnibusTo); } function _grantTransferredACL(euint64 transferred, address omnibusFrom, address omnibusTo) internal { FHE.allowThis(transferred); FHE.allow(transferred, omnibusFrom); FHE.allow(transferred, omnibusTo); }Optionally replace Lines 201–203 with
_grantTransferredACL(transferred, omnibusFrom, omnibusTo);.Also applies to: 193-200
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contracts/token/ERC7984/extensions/ERC7984Omnibus.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-15T14:43:25.623Z
Learnt from: arr00
PR: OpenZeppelin/openzeppelin-confidential-contracts#186
File: contracts/token/ERC7984/extensions/ERC7984Omnibus.sol:140-167
Timestamp: 2025-09-15T14:43:25.623Z
Learning: In ERC7984Omnibus callback functions like confidentialTransferFromAndCallOmnibus, the encrypted sender and recipient addresses are not passed to the callback recipient - only the standard transfer parameters (omnibusFrom, omnibusTo, amount, data) are passed. The ACL grants for the encrypted addresses are for omnibus event emission and future access, not for callback usage.
Applied to files:
contracts/token/ERC7984/extensions/ERC7984Omnibus.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: boostsecurity - boostsecurityio/semgrep-pro
- GitHub Check: Sourcery review
- GitHub Check: tests
- GitHub Check: slither
- GitHub Check: coverage
Summary by Sourcery
Introduce an omnibus confidential transfer extension for ERC7984 and add corresponding mock, tests, and helpers.
New Features:
Tests:
Chores:
Summary by CodeRabbit
New Features
Tests
Documentation
Chores