Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remarks #1

Open
PradhumnaPancholi opened this issue Aug 5, 2022 · 0 comments
Open

Remarks #1

PradhumnaPancholi opened this issue Aug 5, 2022 · 0 comments

Comments

@PradhumnaPancholi
Copy link

  1. Please give names to the parameters in your Stake event. It makes it easy to understand your code, its purpose, and will be helpful for indexing (like creating a subgraph).
  2. Good practice to always set initializing values even when opting for default ones (openForWithdraw state variable)
  3. Your execute function can be more optimized if you check for openForWithdraw. As we know that execute has been already performed if the variable is true.
function execute() public notCompleted {
    //if excuted was performed succesfully before, show an error to reflect this//
    require(!openForWithdraw, "Actions has been already performed!");
    // if deadline is not passed, show error to refelct it//
    require(block.timestamp > deadline, "Deadline is not completed yet!");
    // if is used instead of require becasu we have a counter action to set "openForWithdraw" to true//�
    if(address(this).balance >= threshold) {
      exampleExternalContract.complete{value: address(this).balance}();
    } else {
      // If the `threshold` was not met, allow everyone to call a `withdraw()` function
      openForWithdraw = true;
    }
  }
  1. Bug in withdraw function. You are not updating the balances which will allow user to withdraw multiple times.
  2. Your modifiers must be above functions to follow best practices and conventions.
  3. Good job on deploying and verifying the contract. Please try to link them in readme.
  4. Lack of comments. Please add comments with @notice, @params, etc to make your code easy to understand
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

No branches or pull requests

1 participant