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

Signature voting PR #58

Open
wants to merge 7 commits into
base: signature-voting
Choose a base branch
from

Conversation

gotnoshoeson
Copy link

Description

Build the signature-voting branch

Additional Information

Related Issues

N/A

Note: If your changes are small and straightforward, you may skip the creation of an issue beforehand and remove this section. However, for medium-to-large changes, it is recommended to have an open issue for discussion and approval prior to submitting a pull request.

Your ENS/address: milespatterson.eth

Copy link
Collaborator

@escottalexander escottalexander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving some comments relating to styles and code structure. Feel free to ping me again when you have some working tests.

@@ -28,7 +27,7 @@ foundryup

## Challenge Description
*--Edit this section--*
Write challenge description here...
Voters can sign messages off chain. The sender of a 'vote' transaction may not be the wallet address that signed the message. So, we need a way to get the signer of a signed message in order to record their vote for the correct proposal.

Here are some helpful references:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure you remove these OR even better, add some references for the user to read.

@@ -28,7 +27,7 @@ foundryup

## Challenge Description
*--Edit this section--*
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this line

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And any other lines with *--blah blah--*

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure you pull the latest from main branch. It will make some changes to the readme.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The contract structure should match this:

// Layout of Contract:
  // version
  // imports

// Inside the contract declaration: 
  // errors
  // interfaces, libraries, contracts
  // Type declarations
  // State variables
  // Events
  // Modifiers
  // Functions

// Layout of Functions:
  // constructor
  // receive function (if exists)
  // fallback function (if exists)
  // external
  // public
  // internal
  // private
  // internal & private view & pure functions
  // external & public view & pure functions

Let's also designate the different sections with large block comments like this:

    ///////////////////
    // Public Functions
    ///////////////////

Lastly, use custom errors.

Proposal[] public proposals;

// Creates a proposal
function createProposal (string memory proposalName) external {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every function should have NatSpec comments with a requirements section that tells the user what functionality is expected from the function. See the other completed challenges for an example.

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