Skip to content

Merge pull request #1 from NethermindEth/feat/FixEngine#1

Open
dhruv035 wants to merge 42 commits intoCMTA:mainfrom
NethermindEth:main
Open

Merge pull request #1 from NethermindEth/feat/FixEngine#1
dhruv035 wants to merge 42 commits intoCMTA:mainfrom
NethermindEth:main

Conversation

@dhruv035
Copy link

Add engine with Data module and basic tests

@dhruv035 dhruv035 marked this pull request as draft February 16, 2026 09:11
@ADCDIII
Copy link

ADCDIII commented Feb 20, 2026

Hello @dhruv035,

Great, thanks a lot for your work!

I didn't have the time yet to check all the code, but I just want to highlight that we have released CMTAT v3.2.0 today.

So it could be great if you can update the submodule to v3.2.0 in your project too!

Best

swapnilraj and others added 5 commits February 23, 2026 12:49
Fix engine binding validation and secure token helper descriptor writes, add regression tests for both paths, and include Cursor worktree setup commands for submodule-aware initialization.

Co-authored-by: Cursor <cursoragent@cursor.com>
Add deterministic verifyField assertions, missing engine setter and helper authorization revert-path tests, and refactor repeated descriptor fixtures for clearer, maintainable test suites.

Co-authored-by: Cursor <cursoragent@cursor.com>
Validate initializer-provided engine addresses against the token binding invariant and add dedicated module tests to cover valid, invalid, and zero-engine initialization paths.

Co-authored-by: Cursor <cursoragent@cursor.com>
Use TOKEN_ADDRESS from environment instead of defaulting to implementation address and document the new variable to reduce production misconfiguration risk.

Co-authored-by: Cursor <cursoragent@cursor.com>
Bump the CMTAT submodule to v3.2.0, align base-module imports and Engine constructor usage with the upstream API changes, and refresh dependency documentation to match the current releases.

Co-authored-by: Cursor <cursoragent@cursor.com>
@rya-sge
Copy link
Collaborator

rya-sge commented Feb 23, 2026

Another potential improvement: separate FixDescriptorEngine in two contracts: FixDescriptorEngineand FixDescriptorEngineBase

FixDescriptorEngineBasewill contain all the code from the previous version of FixDescriptorEngine except specific Access Control functions such ashasRole`and the grantRole inside the constructor

This will make easier in the future to use a different access control for the FixDescriptorEngine

Clarify token-side authorization wording to match the hook-based engine policy and correct the repository license reference to MPL-2.0.

Made-with: Cursor
Align module imports and constructor engine struct usage with upstream interface changes so the branch compiles and the full Forge test suite passes again.

Made-with: Cursor
Move token binding and descriptor mechanics into a new base contract with _authorize hooks, keep AccessControl policy in the concrete wrapper, and add base-level harness tests to preserve behavior.

Made-with: Cursor
@swapnilraj
Copy link

swapnilraj commented Feb 27, 2026

Another potential improvement: separate FixDescriptorEngine in two contracts: FixDescriptorEngineand FixDescriptorEngineBase

FixDescriptorEngineBasewill contain all the code from the previous version of FixDescriptorEngine except specific Access Control functions such ashasRole`and the grantRole inside the constructor

Implemented this change, @rya-sge could you help un-draft this PR

@rya-sge rya-sge marked this pull request as ready for review February 27, 2026 14:43
@swapnilraj
Copy link

@rya-sge I think PR is ready to go through a review, lmk if you need anything else from me.

@rya-sge
Copy link
Collaborator

rya-sge commented Mar 11, 2026

Hello @swapnilraj, @dhruv035

Sorry for the delay! Thanks a lot for your work.

Directory

Could it be possible to separate more clearly the CMTAT modified example and associated module and the Engine in the repository ? Also, the Engine should work with any tokens implementing the right interface, I think this is the case but could be relevant to indicate in the readme

PR
I made a small PR, let me know if it sounds good and can be merge in your repository or if changes are needed
NethermindEth#4
Also, can you check the feedback on the Aderyn report ? (see readme)

Warning
I have seen there is a list of warning when I build with Foundry.

Could be relevant to fix them or to explain why they are not fixed.

note[screaming-snake-case-const]: constants should use SCREAMING_SNAKE_CASE --> src/FixDescriptorEngineModule.sol:17:30 | 17 | bytes32 private constant FixDescriptorEngineModuleStorageLocation = | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: FIX_DESCRIPTOR_ENGINE_MODULE_STORAGE_LOCATION`
|
= help: https://book.getfoundry.sh/reference/forge/forge-lint#screaming-snake-case-const

note[screaming-snake-case-immutable]: immutables should use SCREAMING_SNAKE_CASE
--> src/FixDescriptorEngineBase.sol:16:30
|
16 | address public immutable token;
| ^^^^^ help: consider using: TOKEN
|
= help: https://book.getfoundry.sh/reference/forge/forge-lint#screaming-snake-case-immutable

note[mixed-case-function]: function names should use mixedCase
--> src/FixDescriptorEngine.sol:50:14
|
50 | function _authorizeSetFixDescriptorWithSBE() internal virtual override {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: _authorizeSetFixDescriptorWithSbe
|
= help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-function

note[mixed-case-function]: function names should use mixedCase
--> src/FixDescriptorEngineModule.sol:36:14
|
36 | function _FixDescriptorEngineModule_init_unchained(address engine) internal virtual onlyInitializing {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: _fixDescriptorEngineModuleInitUnchained
|
= help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-function

note[mixed-case-variable]: mutable variables should use mixedCase
--> test/FixDescriptorEngine.t.sol:21:18
|
21 | bytes public sampleSBEData = hex"a2011901f70266555344";
| ^^^^^^^^^^^^^ help: consider using: sampleSbeData
|
= help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-variable

note[mixed-case-variable]: mutable variables should use mixedCase
--> src/FixDescriptorEngineBase.sol:61:24
|
61 | bytes calldata pathCBOR,
| ^^^^^^^^ help: consider using: pathCbor
|
= help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-variable

note[mixed-case-function]: function names should use mixedCase
--> test/FixDescriptorEngineBase.t.sol:14:14
|
14 | function _authorizeSetFixDescriptorWithSBE() internal pure override {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: _authorizeSetFixDescriptorWithSbe
|
= help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-function

note[mixed-case-variable]: mutable variables should use mixedCase
--> test/CMTATWithFixDescriptor.t.sol:27:18
|
27 | bytes public sampleSBEData = hex"a2011901f70266555344"; // Simple SBE encoded data
| ^^^^^^^^^^^^^ help: consider using: sampleSbeData
|
= help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-variable

note[mixed-case-variable]: mutable variables should use mixedCase
--> src/examples/CMTATWithFixDescriptor.sol:59:24
|
59 | bytes calldata pathCBOR,
| ^^^^^^^^ help: consider using: pathCbor
|
= help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-variable

note[mixed-case-variable]: mutable variables should use mixedCase
--> test/FixDescriptorEngineBase.t.sol:21:18
|
21 | bytes public sampleSBEData = hex"a2011901f70266555344";
| ^^^^^^^^^^^^^ help: consider using: sampleSbeData
|
= help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-variable

note[mixed-case-variable]: mutable variables should use mixedCase
--> src/modules/FixDescriptorModule.sol:54:24
|
54 | bytes calldata pathCBOR,
| ^^^^^^^^ help: consider using: pathCbor
|
= help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-variable

note[mixed-case-function]: function names should use mixedCase
--> src/FixDescriptorEngineBase.sol:75:14
|
75 | function getFixSBEChunk(uint256 start, uint256 size) external view returns (bytes memory chunk) {
| ^^^^^^^^^^^^^^ help: consider using: getFixSbeChunk
|
= help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-function

note[mixed-case-function]: function names should use mixedCase
--> src/examples/CMTATWithFixDescriptor.sol:75:14
|
75 | function getFixSBEChunk(uint256 start, uint256 size) external view returns (bytes memory chunk) {
| ^^^^^^^^^^^^^^ help: consider using: getFixSbeChunk
|
= help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-function

note[mixed-case-variable]: mutable variables should use mixedCase
--> test/FixDescriptorEngineBase.t.sol:79:22
|
79 | bytes memory pathCBOR = hex"01";
| ^^^^^^^^ help: consider using: pathCbor
|
= help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-variable

note[mixed-case-function]: function names should use mixedCase
--> src/modules/FixDescriptorModule.sol:68:14
|
68 | function _getSBEChunk(uint256 start, uint256 size) internal view returns (bytes memory chunk) {
| ^^^^^^^^^^^^ help: consider using: _getSbeChunk
|
= help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-function

note[mixed-case-variable]: mutable variables should use mixedCase
--> test/FixDescriptorEngine.t.sol:46:22
|
46 | bytes memory emptySBE = "";
| ^^^^^^^^ help: consider using: emptySbe
|
= help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-variable

note[mixed-case-function]: function names should use mixedCase
--> src/examples/CMTATWithFixDescriptor.sol:122:14
|
122 | function setDescriptorWithSBE(bytes memory sbeData, IFixDescriptor.FixDescriptor memory descriptor)
| ^^^^^^^^^^^^^^^^^^^^ help: consider using: setDescriptorWithSbe
|
= help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-function

note[mixed-case-variable]: mutable variables should use mixedCase
--> test/CMTATWithFixDescriptor.t.sol:91:22
|
91 | bytes memory emptySBE = "";
| ^^^^^^^^ help: consider using: emptySbe
|
= help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-variable

note[mixed-case-function]: function names should use mixedCase
--> src/FixDescriptorEngineBase.sol:94:14
|
94 | function setFixDescriptorWithSBE(bytes memory sbeData, IFixDescriptor.FixDescriptor memory descriptor)
| ^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: setFixDescriptorWithSbe
|
= help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-function

note[mixed-case-function]: function names should use mixedCase
--> src/modules/FixDescriptorModule.sol:153:14
|
153 | function _deploySBE(bytes memory data) internal returns (address ptr) {
| ^^^^^^^^^^ help: consider using: _deploySbe
|
= help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-function

note[mixed-case-variable]: mutable variables should use mixedCase
--> test/CMTATWithFixDescriptor.t.sol:187:22
|
187 | bytes memory pathCBOR = hex"01";
| ^^^^^^^^ help: consider using: pathCbor
|
= help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-variable

note[mixed-case-variable]: mutable variables should use mixedCase
--> test/FixDescriptorEngine.t.sol:57:22
|
57 | bytes memory emptySBE = "";
| ^^^^^^^^ help: consider using: emptySbe
|
= help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-variable

note[mixed-case-variable]: mutable variables should use mixedCase
--> test/CMTATWithFixDescriptor.t.sol:235:22
|
235 | bytes memory pathCBOR = hex"01";
| ^^^^^^^^ help: consider using: pathCbor
|
= help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-variable

note[mixed-case-function]: function names should use mixedCase
--> src/FixDescriptorEngineBase.sol:106:14
|
106 | function _authorizeSetFixDescriptorWithSBE() internal virtual;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: _authorizeSetFixDescriptorWithSbe
|
= help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-function

note[mixed-case-variable]: mutable variables should use mixedCase
--> test/FixDescriptorEngine.t.sol:65:22
|
65 | bytes memory emptySBE = "";
| ^^^^^^^^ help: consider using: emptySbe
|
= help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-variable

note[mixed-case-variable]: mutable variables should use mixedCase
--> test/CMTATWithFixDescriptor.t.sol:307:22
|
307 | bytes memory newSBEData = hex"deadbeef";
| ^^^^^^^^^^ help: consider using: newSbeData
|
= help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-variable

note[mixed-case-variable]: mutable variables should use mixedCase
--> test/FixDescriptorEngine.t.sol:103:17
|
103 | address preDeployedSBE = address(0x1234567890123456789012345678901234567890);
| ^^^^^^^^^^^^^^ help: consider using: preDeployedSbe
|
= help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-variable

note[mixed-case-variable]: mutable variables should use mixedCase
--> test/CMTATWithFixDescriptor.t.sol:344:22
|
344 | bytes memory pathCBOR = hex"01";
| ^^^^^^^^ help: consider using: pathCbor
|
= help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-variable

note[mixed-case-variable]: mutable variables should use mixedCase
--> test/FixDescriptorEngine.t.sol:203:22
|
203 | bytes memory newSBEData = hex"deadbeef";
| ^^^^^^^^^^ help: consider using: newSbeData
|
= help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-variable

note[mixed-case-variable]: mutable variables should use mixedCase
--> test/CMTATWithFixDescriptor.t.sol:372:22
|
372 | bytes memory pathCBOR = hex"01";
| ^^^^^^^^ help: consider using: pathCbor
|
= help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-variable

note[mixed-case-variable]: mutable variables should use mixedCase
--> test/FixDescriptorEngine.t.sol:339:22
|
339 | bytes memory pathCBOR = hex"01";
| ^^^^^^^^ help: consider using: pathCbor
|
= help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-variable

note[mixed-case-variable]: mutable variables should use mixedCase
--> test/FixDescriptorEngine.t.sol:362:22
|
362 | bytes memory pathCBOR = hex"01";
| ^^^^^^^^ help: consider using: pathCbor
|
= help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-variable
`

- Move engine contracts to src/engine/ (FixDescriptorEngine, Base, interfaces, modules)
- Move example token to src/example/ (CMTATWithFixDescriptor)
- Keep FixDescriptorEngineModule in src/ for clear separation (engine vs CMTAT integration)
- Update FixDescriptorEngine to use AccessControlEnumerable (align with PR #4 tests)
- Apply CEI reorder in FixDescriptorEngineModule: state update before external call
  in setFixDescriptorEngine and __fixDescriptorEngineModuleInitUnchained (Aderyn H-2)
- Update README project structure and all imports (tests, script)

Made-with: Cursor
- Add .github/workflows/lint.yml: run forge lint, filter SBE/CBOR findings from log
  (Solidity style guide + FixDescriptorKit use caps for initialisms)
- foundry.toml: exclude asm-keccak256 lint (optional micro-opt, keep keccak256())
- Re-run Aderyn, update doc/audit/tools/aderyn-report.md
- Remove unused IFixDescriptorEngine import from CMTATWithFixDescriptor.sol

Made-with: Cursor
- Point inheritance, graph, and report scripts to doc/surya/
- Fix find usage (use SRC_DIR, -name '*.sol') in all three scripts
- Post-process report .md to strip repo root so File Name is relative
- Regenerate inheritance/graph PNGs and report markdown

Made-with: Cursor
- Clarify engine vs CMTAT integration layout and locations
- Move SBE/CBOR and lint notes into Foundry configuration section
- Add lint.yml to project structure; document asm-keccak256 exclusion
- Aderyn: 1 High / 5 Low, remove H-2 (CEI fixed), tidy finding table

Made-with: Cursor
- Add comment in _authorizeSetDescriptorEngine() body (CMTATWithFixDescriptor)
- Remove L-2 Empty Block from aderyn-report; renumber lows to L-2–L-4
- Update feedback doc: Empty Block fixed, summary and section IDs
- README: 1 High / 4 Low, update finding summary table

Made-with: Cursor
@swapnilraj
Copy link

swapnilraj commented Mar 11, 2026

Hello @swapnilraj, @dhruv035

Sorry for the delay! Thanks a lot for your work.

np @rya-sge!

Directory

Could it be possible to separate more clearly the CMTAT modified example and associated module and the Engine in the repository ? Also, the Engine should work with any tokens implementing the right interface, I think this is the case but could be relevant to indicate in the readme

Done! let me know if the new folder structure looks more readable to you.

PR I made a small PR, let me know if it sounds good and can be merge in your repository or if changes are needed NethermindEth#4 Also, can you check the feedback on the Aderyn report ? (see readme)

Merged and fixed the feedback.

Warning I have seen there is a list of warning when I build with Foundry.

Could be relevant to fix them or to explain why they are not fixed.

We should leave CBOR and SBE as it is, because that's how they are used in their normal usage
I've added a comment in the README about and I've also added a CI lint run that ignores the SBE and CBOR complains but keeps all other ones.
official specs: SBE, CBOR.

@rya-sge
Copy link
Collaborator

rya-sge commented Mar 16, 2026

Great! I think we are almost done. May two small last points:

a) If still relevant, potentially add an hypertext to this website: https://fixdescriptor.vercel.app/spec

Also could be nice to add more schema or explanation on how FIX works alongside CMTAT in the readme. Maybe also a terminology section as the one present in the spec

b) After the initial introduction in the readme, if you want, you can add a line like this:
This project was initially developed by Nethermind (swapnilraj, @dhruv035 ) in collaboration with CMTA and Taurus.

See

=> Update link with the correct link if you copy/past

…erminology

- Add credits line (Nethermind, swapnilraj, CMTA, Taurus)
- Add spec link; How FIX messages identify assets with ASCII diagram
- Add Terminology table; caption clarifying contract stores root+SBE, verify is separate
- Note pathCBOR in Terminology; add FIX Descriptor Spec to References

Made-with: Cursor
@swapnilraj
Copy link

Great! I think we are almost done. May two small last points:

a) If still relevant, potentially add an hypertext to this website: https://fixdescriptor.vercel.app/spec

Also could be nice to add more schema or explanation on how FIX works alongside CMTAT in the readme. Maybe also a terminology section as the one present in the spec

Added a simple "layman" explanation with an ascii diagram and link to the spec.

b) After the initial introduction in the readme, if you want, you can add a line like this: This project was initially developed by Nethermind (swapnilraj, @dhruv035 ) in collaboration with CMTA and Taurus.

Done

See

=> Update link with the correct link if you copy/past

I used the vercel link because that's more stable the erc-fix.com link might change once we turn it into an ERC and get a number.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants