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

Fixes: refactored election solidity file and fixed election starting logic #89

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

AdityaInnovates
Copy link

The pull request is regarding the issue: #88

1. Incorrect Modifier Logic (electionStarted)

  • Issue: The electionStarted modifier is supposed to ensure the election has started, but its logic is incorrect:
    if (block.timestamp > electionInfo.startTime) revert ElectionInactive();
    • Corrected: This should check if the current time is before the start time:
      if (block.timestamp < electionInfo.startTime) revert ElectionInactive();

2. Unused Candidate ID (Candidate Struct)

  • Issue: The Candidate struct includes candidateID, which is commented to be unnecessary, yet still used:
    struct Candidate {
        uint candidateID; // remove candidateId its not needed
        string name;
        string description;
    }
    • Corrected: Remove candidateID from the struct if it’s truly unnecessary. The candidates index in the the candidates array can serve as a unique identifier.
    struct Candidate {
        string name;
        string description;
    }

@Ronnieraj37
Copy link
Contributor

Hey @AdityaInnovates the Modifier

modifier electionStarted() {
        if (block.timestamp > electionInfo.startTime) revert ElectionInactive();
        _;
    }

is being used in functions Add / Remove Candidate. Thus the check is to ensure that we don't remove the candidate after the election starts as this might cause multiple problems.

@AdityaInnovates
Copy link
Author

AdityaInnovates commented Jan 9, 2025

Okay, got it, I have fixed the logic. @Ronnieraj37
The PR is ready to be merged now.

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.

2 participants