From e9aee2859b23dc76a6f6cfbe289dcc5b8a3df6a6 Mon Sep 17 00:00:00 2001 From: Kevin Halliday Date: Fri, 23 Aug 2024 15:45:25 -0400 Subject: [PATCH] chore: cleanup comments --- README.md | 8 ++--- src/XStakeController.sol | 27 +++++++-------- src/XStaker.sol | 71 ++++++++++++++++++++-------------------- 3 files changed, 51 insertions(+), 55 deletions(-) diff --git a/README.md b/README.md index 9e22469..0579254 100644 --- 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 index 68eff41..9616712 100644 --- 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 index 0621956..c3ac566 100644 --- 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) {