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 EIP-6065: Move to Review #6973

Merged
merged 41 commits into from
May 20, 2023

Conversation

Alex-Klasma
Copy link
Contributor

Change to Review status

@Alex-Klasma Alex-Klasma requested a review from eth-bot as a code owner May 1, 2023 23:59
@github-actions github-actions bot added c-status Changes a proposal's status s-review This EIP is in Review t-erc labels May 1, 2023
@eth-bot
Copy link
Collaborator

eth-bot commented May 2, 2023

✅ All reviewers have approved.

@eth-bot eth-bot changed the title Eip real estate standard token Update EIP-6065: Move to Review May 2, 2023
@eth-bot eth-bot added the e-review Waiting on editor to review label May 2, 2023
@dfalloncyr
Copy link

Hi @poojaranjan I reviewed the office hours meeting and did not see EIP6065 come up. Was it added for the next meeting? Thanks!

@poojaranjan
Copy link
Contributor

poojaranjan commented May 2, 2023

We couldn't review the proposal due to limited time. I hope it is reviewed and merged async. If it is open until the next meeting, please include it to the agenda.

@dfalloncyr
Copy link

Sounds good, will keep and eye on it. Thanks! Assuming the next meeting is 5/16?

Copy link
Contributor

@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.

Things that need to be fixed:

  • EIP6065Immutable comment (Lines 57 - 70) should be removed. While we do allow some suggestions for implementers in complex cases, I don't think this particular comment adds significant value.
  • Similarly the EIP6065Mutable bits of that comment should be removed.
  • Define RWA before its first use.
  • Foreclosure needs to be defined outside of a comment. What does that mean in terms of the token? Is it forcibly transferred to a different owner? Destroyed?
  • The debt stuff is under-specified. If the debt is a token, how is it "utilized specifically for off-chain debt"?
  • Link to your reference implementation file in the reference implementation section.
  • Remove (*see pull request above*) on line 180.
  • EIPs are not advertisements, so I'd like to see the reference implementation section de-klasma'd a bit. I'm fine with a "Klasma Labs offers a work in progress..." intro sentence, but I'd prefer everything after use fictitious company names.

@Alex-Klasma
Copy link
Contributor Author

Still working on this, just merging some prior stuff

@Alex-Klasma Alex-Klasma dismissed a stale review via 4481775 May 15, 2023 17:25
@Alex-Klasma
Copy link
Contributor Author

Things that need to be fixed:

* `EIP6065Immutable` comment (Lines 57 - 70) should be removed. While we do allow some suggestions for implementers in complex cases, I don't think this particular comment adds significant value.

* Similarly the `EIP6065Mutable` bits of that comment should be removed.

* Define RWA before its first use.

* Foreclosure needs to be defined outside of a comment. What does that mean in terms of the token? Is it forcibly transferred to a different owner? Destroyed?

* The debt stuff is under-specified. If the debt is a token, how is it "utilized specifically for off-chain debt"?

* Link to your reference implementation file in the reference implementation section.

* Remove `(*see pull request above*)` on line 180.

* EIPs are not advertisements, so I'd like to see the reference implementation section de-klasma'd a bit. I'm fine with a "Klasma Labs offers a work in progress..." intro sentence, but I'd prefer everything after use fictitious company names.
  1. Comments in the code about "structs" removed.
  2. removed
  3. defined RWA as "real world assets" now before first usage of abbreviation.
  4. Foreclosure defined in L#126,### debtOf heading
  5. "off chain" debt has been specified further with some examples.
  6. Reference implementation linked
  7. This line has been removed and replaced by the reference implementation link.
  8. All "Klasma" references have been removed except the single line you were OK with -- image also been changed to account for this

@dfalloncyr
Copy link

@SamWilsn sorry we missed the office hours yesterday! Didn't see it mentioned until after it had happened (we are located in California). We have addressed all of your comments above, please let us know if there is anything else that needs to be changed to move this to review and we can address async. Thanks for all of your help!

@eth-bot eth-bot enabled auto-merge (squash) May 20, 2023 05:54
Copy link
Collaborator

@eth-bot eth-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...

@eth-bot eth-bot merged commit 2a4285b into ethereum:master May 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-status Changes a proposal's status e-review Waiting on editor to review s-review This EIP is in Review t-erc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants