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-3643: Move to Draft #7164

Merged
merged 21 commits into from
Jul 3, 2023
Merged

Update EIP-3643: Move to Draft #7164

merged 21 commits into from
Jul 3, 2023

Conversation

Joachim-Lebrun
Copy link
Contributor

@Joachim-Lebrun Joachim-Lebrun commented Jun 11, 2023

  • put EIP-3643 out of stagnant status
  • previous status was draft, so the current PR sets the status back to draft, following recommandations of EIP-1
  • Adapt the content of the EIP to the last audited version of the T-REX protocol
  • Remove useless reference to upgradeable proxies => proxies are used in T-REX but it is up to the developer to decide if they want to make the token upgradeable or not. as long as interfaces are matching the standard it doesn't matter.
  • Remove duplicate functions, e.g. transferOwnershipOnIdentityRegistryContract which is a duplicate of transferOwnership as we mention the ERC-173 used by the standard.
  • Remove all links to external sources, assets added in the repository (pdf files of audits/interface contracts)
  • Remove references to ERC-734 and ERC-735 as these are not official EIPs
  • Fix order of sections and other elements according to EIP-1
  • add reference to ERC-173 instead of implementation recommandation (OZ Ownable library)
  • add the interface of IAgentRole as the role is important in the standard, previously it was described with implementation related reference, now we focus on the externally visible functions of contracts only, not implementation details

put out of stagnant status
@github-actions github-actions bot added c-status Changes a proposal's status s-draft This EIP is a Draft t-erc labels Jun 11, 2023
@eth-bot
Copy link
Collaborator

eth-bot commented Jun 11, 2023

✅ All reviewers have approved.

@eth-bot eth-bot changed the title EIP-3643 : Stagnant => Draft Update EIP-3643: Move to Draft Jun 11, 2023
@eth-bot eth-bot added the e-review Waiting on editor to review label Jun 11, 2023
@Joachim-Lebrun Joachim-Lebrun changed the title Update EIP-3643: Move to Draft Update EIP-3643: Move back to Draft Jun 11, 2023
@github-actions github-actions bot added the w-ci Waiting on CI to pass label Jun 11, 2023
@eth-bot eth-bot changed the title Update EIP-3643: Move back to Draft Update EIP-3643: Move to Draft Jun 11, 2023
@github-actions github-actions bot removed the w-ci Waiting on CI to pass label Jun 11, 2023
@github-actions github-actions bot added the w-ci Waiting on CI to pass label Jun 11, 2023
@github-actions
Copy link

The commit c69bb86 (as a parent of b332759) contains errors.
Please inspect the Run Summary for details.

@github-actions github-actions bot removed the w-ci Waiting on CI to pass label Jun 12, 2023
assets/eip-3643/TREX_V3_audit.pdf Outdated Show resolved Hide resolved
assets/eip-3643/TREX_V4_audit.pdf Outdated Show resolved Hide resolved
EIPS/eip-3643.md Outdated Show resolved Hide resolved
EIPS/eip-3643.md Outdated Show resolved Hide resolved
EIPS/eip-3643.md Outdated Show resolved Hide resolved
EIPS/eip-3643.md Outdated Show resolved Hide resolved
EIPS/eip-3643.md Outdated Show resolved Hide resolved
EIPS/eip-3643.md Outdated Show resolved Hide resolved
EIPS/eip-3643.md Outdated
Comment on lines 409 to 410
The suite of Smart Contracts has been audited by external independent companies. The results can be found on
Tokeny's website or on the Tokeny's T-REX repository.
Copy link
Contributor

Choose a reason for hiding this comment

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

Mentioning your website is conceptually an "external link", and so isn't allowed. I'd recommend either summarizing the results of the audit, or simply noting that no security issues have been found.

Perhaps something like one of these, as appropriate:


This specification has been audited by Kapersky, and no notable security considerations were found.


An audit, performed by Kapersky, highlighted the following considerations for implementers of this standard:

  • When flooping a glarp, implementers must pay particular attention to the meep. It must remain glooped for the duration of the merp.
  • Never transfer the moop to the merp. Doing so would allow an attacker to floop the glarp too early.

From an extremely quick read of the audit report, it looks like it was focused on your implementation, but not on this standard itself. In other words, they focused on whether your implementation conformed to your whitepaper, and didn't consider whether your overall design was sound.

That isn't problematic in and of itself, but it does mean the audit might not be applicable to other implementers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, the audit mainly focused on the concrete implementation of the standard. However, it is important to note that our implementation is a direct translation of the standard specification outlined in the whitepaper. Therefore, the audit process essentially included a thorough review and verification of the standard's design and functionality.

While the audit report may not explicitly state the details of these discussions, the audit process was comprehensive and spanned over four months. During this time, the audit team harshly challenged the principles and design decisions underlying the T-REX standard (ERC-3643). Their inquiry was not limited to the security aspects of smart contracts, but also delved into the rationale behind the standard's specifications. Satisfactory responses to these inquiries and the absence of any outstanding safety issues in the audit report demonstrate the robustness of the standard.

It is true that the audits we realized may not be fully applicable to other implementations of the standard, but still provide valuable insight into the standard's design and functionality. The audits validate the standard's specifications and provide assurance that the standard does not present any notable security issues (when implemented as per the specifications). Therefore, we consider it relevant and useful to mention the audits in this section related to standard security considerations. 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the Security Considerations section in adfc130

Joachim-Lebrun and others added 4 commits June 19, 2023 19:45
Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>
Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>
Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>
@Joachim-Lebrun
Copy link
Contributor Author

@SamWilsn is there still any changes required to merge this PR?

@SamWilsn SamWilsn closed this Jul 3, 2023
@SamWilsn SamWilsn reopened this Jul 3, 2023
@eth-bot eth-bot enabled auto-merge (squash) July 3, 2023 14:47
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 870e33c into ethereum:master Jul 3, 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-draft This EIP is a Draft t-erc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants