chore: cleanup comments

omni-network · Aug 23, 2024 · e9aee28 · e9aee28
1 parent f52e8fb
commit e9aee28
Show file tree

Hide file tree

Showing 3 changed files with 51 additions and 55 deletions.
diff --git a/README.md b/README.md
@@ -18,20 +18,20 @@ The protocol has two contracts
 The first accepts deposits, and pays out withdrawals. The second maintains global accounting, and authorizes withdrawals. To learn how each contract works, read the source code. It's not long, and is commented generously. Read in the following order:
 
 
-1. [`XStaker.stake`](./src/XStaker.sol#L64)
+1. [`XStaker.stake`](./src/XStaker.sol#L65)
 
     Entrypoint for staking. This function accepts deposits, and records them with the `XStakeController` via `xcall`.
 
 
-2. [`XStakeController.recordStake`](./src/XStakeController.sol#L40)
+2. [`XStakeController.recordStake`](./src/XStakeController.sol#L38)
 
     Records stake. Only callable by a known `XStaker` contract on a supported chain.
 
-3. [`XStakeController.unstake`](./src/XStakeController.sol#L55)
+3. [`XStakeController.unstake`](./src/XStakeController.sol#L54)
 
     Entrypoint for unstaking. This function authorizes withdrawals, and directs a payout to the corresponding `XStaker` via `xcall`.
 
-4. [`XStaker.withdraw`](./src/XStaker.sol#L97)
+4. [`XStaker.withdraw`](./src/XStaker.sol#L96)
 
     Withdraws stake back to the user. Only callable by the `XStakeController`.
 

diff --git a/src/XStakeController.sol b/src/XStakeController.sol
@@ -9,10 +9,16 @@ import {XStaker} from "./XStaker.sol";
 
 /**
  * @title XStakeController
+ *
  * @notice The global accountant of our cross-chain staking protocol. This is a
- *         singleton contract, deployed on Omni. It tracks stakes for all users
- *         across each supported chain, and directs the XStaker to payout users
- *         when they unstake.
+ *      singleton contract, deployed on Omni. It tracks stakes for all users
+ *      across each supported chain, and directs the XStaker to payout users
+ *      when they unstake.
+ *
+ * @dev We initialize our XApp with default ConfLevel.Finalized (see constructor),
+ *      and do not specify conf level in individual xcalls, as we do in XStaker.
+ *      This is bescause XStakeController is deployed on Omni. Omni only
+ *      supports Finalized conf level, as OmniEVM has instant finality.
  */
 contract XStakeController is XApp, Ownable {
     /// @notice Address of XStaker contract by chain id.
@@ -21,14 +27,6 @@ contract XStakeController is XApp, Ownable {
     /// @notice Map account to chain id to stake.
     mapping(address => mapping(uint64 => uint256)) public stakeOn;
 
-    /**
-     * @notice Constructor.
-     *
-     *  NOTE: We initialize our XApp with default ConfLevel.Finalized, and do not
-     *  specify conf level in individual xcalls, as we do in XStaker.
-     *  This is bescause XStakeController is deployed on Omni. Omni only
-     *  supports Finalized conf level, as OmniEVM has instant finality.
-     */
     constructor(address portal, address owner) XApp(portal, ConfLevel.Finalized) Ownable(owner) {}
 
     /**
@@ -47,10 +45,9 @@ contract XStakeController is XApp, Ownable {
 
     /**
      * @notice Unstake msg.sender `onChainID`.
-     *
-     * NOTE: Unstaking starts on the controller, because the controller is the
-     * source of truth for user stakes. The controller directs the XStaker to
-     * payout the user via xcall.
+     * @dev Unstaking starts on the controller, because the controller is the
+     *      source of truth for user stakes. The controller directs the XStaker to
+     *      payout the user via xcall.
      */
     function unstake(uint64 onChainID) external payable {
         uint256 stake = stakeOn[msg.sender][onChainID];

diff --git a/src/XStaker.sol b/src/XStaker.sol
@@ -9,10 +9,41 @@ import {XStakeController} from "./XStakeController.sol";
 
 /**
  * @title XStaker
+ *
  * @notice Deployed on multiple chains, this contract is the entry / exit point for
- *         our cross-chain staking protocol. It accepts ERC20 deposits, and records
- *         them with the XStakeController on Omni. When a user unstakes on Omni,
- *         this contract pays out the widrawal.
+ *      our cross-chain staking protocol. It accepts ERC20 deposits, and records
+ *      them with the XStakeController on Omni. When a user unstakes on Omni,
+ *      this contract pays out the widrawal.
+ *
+ * @dev Here we allow the user to specify the confirmation level for the xcall.
+ *      The options are:
+ *
+ *      - ConfLevel.Finalized
+ *          Strongest security guarantee - the xcall will be delivered exactly once.
+ *
+ *          It is only confirmed and relayed when the source tx is finalized.
+ *          L2 txs are finalized only when posted to L1, and the corresponding
+ *          L1 block is finalized. This can take 5-20 minutes.
+ *
+ *      - ConfLevel.Latest
+ *          Weaker security guarantees - best effort delivery.
+ *
+ *          These xcalls are subject to reorg risk. It is possible that
+ *          1. The xcall is delivered, but excluded from the canonical source chain.
+ *          2. The xcall is included in the canonical source chain, but not delivered.
+ *
+ *          In scenario 1, our XStakeController will view this user as staked, but
+ *          the user will still cutsody their tokens on the source.
+ 8
+ *          In scenario 2, the XStaker will take cutsody of the user's tokens, but
+ *          the XStakeController will not recognize the user as staked.
+ *
+ *      We can choose to reduce protocol risk, by requiring ConfLevel.Finalized for deposits
+ *      over a certain maximum. Or, there are alternative protocol designs that can mitigate
+ *      the impact of reorgs, and offer recovery mechanisms.
+ *
+ *      Finally, note that current reorg risk on today's L2s (like Optimism and Arbitrum)
+ *      is low. Though it should remain a consideration.
  */
 contract XStaker is XApp {
     /// @notice Stake token.
@@ -30,36 +61,6 @@ contract XStaker is XApp {
      * @notice Stakes `amount` tokens.
      * @param amount    Amount of tokens to stake.
      * @param confLevel XCall confirmation level
-     *
-     * NOTE: Here we allow the user to specify the confirmation level for the xcall.
-     * The options are:
-     *
-     *  - ConfLevel.Finalized
-     *      Strongest security guarantee - the xcall will be delivered exactly once.
-     *
-     *      It is only confirmed and relayed when the source tx is finalized.
-     *      L2 txs are finalized only when posted to L1, and the corresponding
-     *      L1 block is finalized. This can take 5-20 minutes.
-     *
-     *  - ConfLevel.Latest
-     *      Weaker security guarantees - best effort delivery.
-     *
-     *      These xcalls are subject to reorg risk. It is possible that
-     *       1. The xcall is delivered, but excluded from the canonical source chain.
-     *       2. The xcall is included in the canonical source chain, but not delivered.
-     *
-     *      In scenario 1, our XStakeController will view this user as staked, but the user
-     *      will still cutsody their tokens on the source.
-     *
-     *      In scenario 2, the XStaker will take cutsody of the user's tokens, but the
-     *      XStakeController will not recognize the user as staked.
-     *
-     * We can choose to reduce protocol risk, by requiring ConfLevel.Finalized for deposits
-     * over a certain maximum. Or, there are alternative protocol designs that can mitigate
-     * the impact of reorgs, and offer recovery mechanisms. I plan to add an example of that here.
-     *
-     * Finally, I'll note that current reorg risk on today's L2s (like Optimism and Arbitrum)
-     * is low. Though it should remain a consideration.
      */
     function stake(uint256 amount, uint8 confLevel) external payable {
         require(amount > 0, "XStaker: insufficient amount");
@@ -73,13 +74,11 @@ contract XStaker is XApp {
             gasLimit: GasLimits.RecordStake
         });
 
-        // The call above will revert if address(this).balance < fee OR msg.value < fee
-        // This check ensures that the user pays the fee (msg.value >= fee)
+        // Make sure the user paid
         require(msg.value >= fee, "XStaker: insufficient fee");
     }
 
     /**
-     * /**
      * @notice Returns the xcall fee for required to stake.
      */
     function stakeFee(uint256 amount) public view returns (uint256) {