update comments

comment

comment

better error

comment

Author: LeoPatOZ

src/protocol/CheckpointTracker.sol

@@ -7,15 +7,14 @@ import {IPublicationFeed} from "./IPublicationFeed.sol";
 import {IVerifier} from "./IVerifier.sol";
 
 contract CheckpointTracker is ICheckpointTracker {
-    /// @notice The publication id of the current proven checkpoint representing the latest verified state of the rollup
-    /// @dev A checkpoint commitment is any value (typically a state root) that uniquely identifies
-    /// the state of the rollup at a specific point in time
+    /// @dev The publication id of the current proven checkpoint representing
+    /// the latest verified state of the rollup
     uint256 private _provenPublicationId;
 
     IPublicationFeed public immutable publicationFeed;
     IVerifier public immutable verifier;
     ICommitmentStore public immutable commitmentStore;
-    address public proverManager;
+    address public immutable proverManager;
 
     /// @param _genesis the checkpoint commitment describing the initial state of the rollup
     /// @param _publicationFeed the input data source that updates the state of this rollup
@@ -73,11 +72,16 @@ contract CheckpointTracker is ICheckpointTracker {
         _updateCheckpoint(end.publicationId, end.commitment);
     }
 
+    /// @inheritdoc ICheckpointTracker
     function getProvenCheckpoint() public view returns (Checkpoint memory provenCheckpoint) {
         provenCheckpoint.publicationId = _provenPublicationId;
         provenCheckpoint.commitment = commitmentStore.commitmentAt(address(this), provenCheckpoint.publicationId);
     }
 
+    /// @dev Updates the proven checkpoint to a new publication ID and commitment
+    /// @dev Stores the commitment in the commitment store and emits an event
+    /// @param publicationId The ID of the publication to set as the latest proven checkpoint
+    /// @param commitment The checkpoint commitment representing the state at the given publication ID
     function _updateCheckpoint(uint256 publicationId, bytes32 commitment) internal {
         _provenPublicationId = publicationId;
         commitmentStore.storeCommitment(publicationId, commitment);

src/protocol/ICommitmentStore.sol

@@ -15,12 +15,13 @@ interface ICommitmentStore {
     /// @dev A new `commitment` has been stored by `source` at a specified `height`.
     event CommitmentStored(address indexed source, uint256 indexed height, bytes32 commitment);
 
-    /// @dev Returns the commitment at the given `height`.
+    /// @notice Returns the commitment at the given `height`.
+    /// @dev If the commitment does not exist at the given `height`, it returns zero.
     /// @param source The source address for the saved commitment
     /// @param height The height of the commitment
     function commitmentAt(address source, uint256 height) external view returns (bytes32 commitment);
 
-    /// @dev Stores a commitment.
+    /// @notice Stores a commitment attributed to `msg.sender`
     /// @param height The height of the commitment
     /// @param commitment The commitment to store
     function storeCommitment(uint256 height, bytes32 commitment) external;

src/protocol/ISignalService.sol

@@ -28,6 +28,9 @@ interface ISignalService {
     /// @dev We require the commitment to contain a state root (with an embedded storage root)
     error StorageRootCommitmentNotSupported();
 
+    /// @dev If the commitment returns 0 we assume it does not exist
+    error CommitmentNotFound();
+
     /// @notice Stores a signal and returns its storage location.
     /// @param value Data to be stored (signalled)
     function sendSignal(bytes32 value) external returns (bytes32 slot);
@@ -44,8 +47,9 @@ interface ISignalService {
     /// sent by `sender` on the source chain, which state is represented by `commitmentPublisher` at `height`.
     /// @dev Signals are not deleted when verified, and can be
     /// verified multiple times by calling this function
-    /// @param height This refers to the block number / commitmentId where the trusted root is mapped to
-    /// @param commitmentPublisher The address that published the commitment containing the signal.
+    /// @param height A reference value indicating which trusted root to use for verification
+    /// see ICommitmentStore for more information
+    /// @param commitmentPublisher The address that published the commitment
     /// @param sender The address that originally sent the signal on the source chain
     /// @param value The signal value to verify
     /// @param proof The encoded value of the SignalProof struct

src/protocol/SignalService.sol

@@ -8,14 +8,16 @@ import {ETHBridge} from "./ETHBridge.sol";
 import {ICommitmentStore} from "./ICommitmentStore.sol";
 import {ISignalService} from "./ISignalService.sol";
 
-/// @dev SignalService combines secure cross-chain messaging with native token bridging.
+/// @dev SignalService is used for secure cross-chain messaging
 ///
-/// This contract allows sending arbitrary data as signals via `sendSignal`, verifying signals from other chains using
-/// `verifySignal`, and bridging native ETH with built-in signal generation and verification. It integrates:
-/// - `CommitmentStore` to access state roots,
-/// - `LibSignal` for signal hashing, storage, and verification logic.
+/// This contract allows sending arbitrary data as signals via `sendSignal` and verifying signals from other chains
+/// using`verifySignal`
+///   It integrates:
+///    - `CommitmentStore` to access state roots,
+///    - `LibSignal` for signal hashing, storage, and verification logic.
 ///
-/// Signals stored cannot be deleted and can be verified multiple times.
+/// Signals stored cannot be deleted
+/// WARN: this contract does not provide replay protection(signals can be verified multiple times).
 contract SignalService is ISignalService, CommitmentStore {
     using LibSignal for bytes32;
 
@@ -58,12 +60,20 @@ contract SignalService is ISignalService, CommitmentStore {
         // For now it could be the block hash or other hashed value
         // further work is needed to ensure we get the 'state root' of the chain
         bytes32 root = commitmentAt(commitmentPublisher, height);
+
+        // A 0 root would probably fail further down the line but its better to explicitly check
+        require(root != 0, CommitmentNotFound());
+
         SignalProof memory signalProof = abi.decode(proof, (SignalProof));
         bytes[] memory accountProof = signalProof.accountProof;
         bytes[] memory storageProof = signalProof.storageProof;
-        // if there is no account proof, verify signal will treat root as a storage root
-        // for now, we only support full state roots
+
+        // We only support state roots for verification
+        // this is to avoid state roots being used as storage roots (for safety)
         require(accountProof.length != 0, StorageRootCommitmentNotSupported());
-        value.verifySignal(namespace, sender, root, accountProof, storageProof);
+
+        value.verifySignal(sender, root, accountProof, storageProof);
+
+        emit SignalVerified(sender, value);
     }
 }