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

add CommunityVault contract #22

Merged
merged 5 commits into from
Dec 12, 2023
Merged

add CommunityVault contract #22

merged 5 commits into from
Dec 12, 2023

Conversation

0xb337r007
Copy link
Collaborator

Description

Describe the changes made in your pull request here.

Checklist

Ensure you completed all of the steps below before submitting your pull request:

  • Added natspec comments?
  • Ran forge snapshot?
  • Ran pnpm lint?
  • Ran forge test?

@0xb337r007 0xb337r007 marked this pull request as draft November 20, 2023 08:53
Copy link
Member

@0x-r4bbit 0x-r4bbit left a comment

Choose a reason for hiding this comment

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

Great start!

contracts/CommunityVault.sol Outdated Show resolved Hide resolved
@0xb337r007 0xb337r007 marked this pull request as ready for review November 27, 2023 10:32
Copy link
Member

@0x-r4bbit 0x-r4bbit left a comment

Choose a reason for hiding this comment

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

This LGTM, just some minor comments and I believe this code doesn't manage to get past the linter.

amounts[0] = 1;
amounts[1] = 1;

vm.prank(accounts[0]);
Copy link
Member

Choose a reason for hiding this comment

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

The test contract itself is going to be msg.sender by default, so no vm.prank() needed here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would prefer to have it explicitly written in case we change the way we deploy in the future, wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

That's perfectly fine. Was not an actionable comment.

test/CommunityVault.t.sol Outdated Show resolved Hide resolved
@0x-r4bbit
Copy link
Member

@0xb337r007 this LGTM!

I'd just ask you to squash the commits as we don't need to have the steps where stuff didn't get past the linter etc.

@0xb337r007 0xb337r007 merged commit 6f83e73 into main Dec 12, 2023
6 checks passed
@0xb337r007 0xb337r007 deleted the community-vault branch December 12, 2023 22:10
@0xb337r007
Copy link
Collaborator Author

@0x-r4bbit cool I squashed and merged it

@0xb337r007
Copy link
Collaborator Author

I did it directly here in UI

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