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

Update ERC-4824: Move to Review #330

Merged
merged 29 commits into from
Jun 11, 2024
Merged

Conversation

thelastjosh
Copy link
Contributor

I moved the example contract to the assets folder, made various minor clarifications about how daoURI and indexing should work, supplemented the rationale, changed the id format for members to align more closely with CAIP-10 (already used for proposals), and fixed a bunch of typos. All changes are backward compatible.

@eip-review-bot
Copy link
Collaborator

eip-review-bot commented Mar 19, 2024

✅ All reviewers have approved.

@eip-review-bot eip-review-bot changed the title Move to review status Update ERC-4824: Move to Review Mar 19, 2024
Copy link

The commit 2103dc7 (as a parent of fb4e937) contains errors.
Please inspect the Run Summary for details.

@github-actions github-actions bot added the w-ci label Mar 19, 2024
@github-actions github-actions bot removed the w-ci label Mar 19, 2024
thelastjosh added a commit to metagov/daostar that referenced this pull request Mar 19, 2024
Copy link
Collaborator

@SamWilsn SamWilsn left a comment

Choose a reason for hiding this comment

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

In the "Community Consensus" subsection:

Please write proposals as if they are already final. It doesn't make much sense to say "X will happen in 2022" when the proposal is being read in 2032.

We generally don't do "acknowledgements" in the body of an EIP.

ERCS/erc-4824.md Outdated Show resolved Hide resolved
ERCS/erc-4824.md Outdated Show resolved Hide resolved
ERCS/erc-4824.md Show resolved Hide resolved
ERCS/erc-4824.md Outdated Show resolved Hide resolved
assets/erc-4824/contracts/ERC4824Factory.sol Outdated Show resolved Hide resolved
assets/erc-4824/contracts/ERC4824Factory.sol Outdated Show resolved Hide resolved
@thelastjosh
Copy link
Contributor Author

Responded to all review suggestions, moved the smart contract out into assets. Should be ready to move to review!

@thelastjosh
Copy link
Contributor Author

@SamWilsn @xinbenlv or other EIP editor, just a ping so this change can get finally merged! 🙏

@SamWilsn
Copy link
Collaborator

Put up a pull request with some minor fixes: thelastjosh#3

@thelastjosh
Copy link
Contributor Author

Put up a pull request with some minor fixes: thelastjosh#3

Merged!

@eip-review-bot eip-review-bot enabled auto-merge (squash) June 11, 2024 18:34
Copy link
Collaborator

@eip-review-bot eip-review-bot left a comment

Choose a reason for hiding this comment

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

All Reviewers Have Approved; Performing Automatic Merge...

@eip-review-bot eip-review-bot merged commit 7e42221 into ethereum:master Jun 11, 2024
10 checks passed
@thelastjosh
Copy link
Contributor Author

Thank you!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants