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

[hyungi] Vote Contract 작성 #2

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

[hyungi] Vote Contract 작성 #2

wants to merge 7 commits into from

Conversation

Hyunki6040
Copy link
Contributor

No description provided.

@Hyunki6040 Hyunki6040 changed the title Vote Contract 작성 [hyungi] Vote Contract 작성 Feb 13, 2022
README.md Outdated
require(
msg.sender == owner,
"msg sender is not a owner."
Copy link
Member

Choose a reason for hiding this comment

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

Owner를 체크하는 함수는 Ownable 컨트랙트를 상속받아서 사용하면 편하게 사용할 수 있습니다!
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable.sol

Copy link
Contributor Author

Choose a reason for hiding this comment

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

네! 수정했습니다ㅎㅎ

now >= startTime && now < endTime,
"this vote is not available"
);
Copy link
Member

Choose a reason for hiding this comment

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

유용한 modifier네요 👍

README.md Outdated
if(electedProposal.voteCount >= 0){
uint voteCount = 0;
uint electedIndex = 0; // 투표가 모두 0이면 제일 먼저 등록한 메뉴로 선정 (선착순)
Copy link
Member

Choose a reason for hiding this comment

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

투표가 0일때는 메뉴로 선정하지 않는 것이 맞는 것 같아요.
회의때 투표수가 NFT 홀더의 과반수 이상일 때만 메뉴를 추가하기로 한 것 같습니다!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

네! 수정했습니다ㅎㅎ

for(uint i = 1; i < proposals.length; i++){
string memory temp = string(abi.encodePacked(", ", proposals[i].menu));
result = string(abi.encodePacked(result, temp));
Copy link
Member

@anxiubin anxiubin Feb 15, 2022

Choose a reason for hiding this comment

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

메뉴 목록을 조회하는 함수 좋은 것 같아요👍

// NFT 홀더 추가 함수
function addNFTHolder(address _address) private {
require(
!nftHolders[_address],
Copy link
Member

Choose a reason for hiding this comment

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

nftHolders는 어디서 가져오는걸까요?


address public owner; // 컨트랙트 소유자

mapping(address => Vote) public votes; // 투표한 건수들 모음
Copy link
Member

Choose a reason for hiding this comment

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

proposals와 votes를 따로 관리하는 이유가 있을까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

proposals에는 제안된 메뉴별 투표수만 담고 있고 votes는 중복 투표를 막기위한 투표자 명단입니다!

proposals에 투표자 명단을 넣지 않은 이유는 사용자들이 어떤것에 투표했는 지, 비밀 투표를 유지하기 위함과 중복 투표인지 확인할 때 일일이 각각의 proposals에 확인하지 않고 votes만 참조하도록 하기 위해서 분리하기 위함도 있어요!

Copy link

@sapcy sapcy left a comment

Choose a reason for hiding this comment

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

컨트랙트 작성하시느라 고생하셨습니다!👍👍

README.md Outdated
require(
msg.sender == owner,
"msg sender is not a owner."
Copy link

Choose a reason for hiding this comment

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

저도 비슷하게 owner 관련 modifier를 직접 만들어서 썼는데 수빈님 리뷰대로 Ownable 컨트랙트를 상속받아서 하는 것이 맞는 듯 합니다ㅎㅎ

README.md Outdated
if(electedProposal.voteCount >= 0){
uint voteCount = 0;
uint electedIndex = 0; // 투표가 모두 0이면 제일 먼저 등록한 메뉴로 선정 (선착순)
Copy link

Choose a reason for hiding this comment

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

투표가 모두 0이면 원하는 사람이 없는 것으로 간주하여 메뉴를 등록하지 않고 투표를 무효로 하는 것이 맞다고 생각합니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

네! 수정했습니다ㅎㅎ

README.md Outdated
"The vote is not ended."
);
if(electedProposal.voteCount >= 0){
Copy link

Choose a reason for hiding this comment

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

electedProposal.voteCount >= 0일때 밑의 로직을 실행하면서 투표 결과를 추첨하게 되는데 vote contract의 함수 중에 electedProposal의 값을 세팅하는 부분이 보이지가 않는 것 같습니다.. 그래서 조건문의 의도를 잘 모르겠습니다!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

null chek를 위해서 넣었던 부분인데 bytes().lengh == 0 으로 변경했습니다!

now >= startTime && now < endTime,
"this vote is not available"
);
Copy link

Choose a reason for hiding this comment

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

깔끔한 modifier입니다!👍

votes[msg.sender] = Vote({
vote: proposalIndex
});
Copy link

Choose a reason for hiding this comment

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

votes 매핑을 통해 메뉴 투표 했는지 안했는지 여부를 알 수 있게끔 한 의도는 좋은 것 같습니다!
다만 투표가 끝난 뒤 votes 매핑을 초기화하는 로직을 고민해봐야 될 것 같습니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

네! 수정했습니다ㅎㅎ

@Yunseongpyo
Copy link

고생하셨습니다👍

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.

4 participants