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

20240207 invite as org #87

Merged
merged 4 commits into from
Feb 7, 2024
Merged

20240207 invite as org #87

merged 4 commits into from
Feb 7, 2024

Conversation

benjaminbollen
Copy link
Member

@benjaminbollen benjaminbollen commented Feb 7, 2024

⚠️ I ll rebase this once the other PR is also on develop, thanks @jaensen / ✅ done

@benjaminbollen benjaminbollen linked an issue Feb 7, 2024 that may be closed by this pull request
@benjaminbollen
Copy link
Member Author

that seems to be now 🎉

function inviteHumanAsOrganization(address _human, address payable _donationReceiver) external payable {
require(msg.value > MINIMUM_DONATION, "Donation must be at least 0.1 xDai.");
require(isOrganization(msg.sender), "Only organizations can invite.");

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
require(msg.sender != _donationReceiver, "Can't donate to yourself");

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes it at least a little bit more expensive to perpetually invite people with the same funds.

Copy link
Member Author

Choose a reason for hiding this comment

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

well, I agree, but you can always send to yourself over one-hop, so I don't know if we should block it. we already know that the donation is a reputation requirement for it to have meaning

Copy link
Member Author

Choose a reason for hiding this comment

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

I added the requirement with comment

src/hub/Hub.sol Outdated

// set the last mint time to the current timestamp for invited human
// and register the v1 Circles contract status
address v1CirclesStatus = _avatarV1CirclesStatus(_human);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's refactor the common parts of inviteHuman and inviteHumanAsOrganization into an _inviteHuman function

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, I wanted to, but then thought I was overthinking it. Ill quickly do

Copy link
Member Author

Choose a reason for hiding this comment

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

turned out it was well worth the refactoring!

Copy link
Contributor

@jaensen jaensen left a comment

Choose a reason for hiding this comment

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

small refactoring

Copy link
Contributor

@jaensen jaensen left a comment

Choose a reason for hiding this comment

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

fipf

@jaensen jaensen merged commit 1d8e333 into develop Feb 7, 2024
1 check passed
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.

inviteHumanAsOrganization as an organization
2 participants