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

ERC20Template4 - Confidential EVM #870

Merged
merged 19 commits into from
Aug 13, 2024

Conversation

alexcos20
Copy link
Member

ERC20TemplateSapphire (asset files object stored in contract)

- follows same functions and principles as ERC20TemplateEnterprise, with the following additions
- should be deployed only on Oasis Sapphire, and all transactions should be encrypted, otherwise security is compromised
- on initialize, files object(asset URLs) is stored in the contract
- owner can change files object anytime, calling setFilesObject
- for every order, consumer and serviceId are added to a mapping
- has a list of allowed/denied providers (using standard ERC721.balanceOf). Contracts addresses can be changed by owner
- when a provider tries to fetch the files object (by calling getFilesObject), the following conditions are checked:
    - provider address has to be in allow list and not on deny list (given those lists are not address_zero)
    - consumer has to have a valid order

@alexcos20 alexcos20 requested a review from trentmc as a code owner August 7, 2024 14:12
@alexcos20 alexcos20 self-assigned this Aug 7, 2024
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Slither found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

Copy link

openzeppelin-code bot commented Aug 7, 2024

Feature/template_sapphire

Generated at commit: 2ed0099a60357a11cf13368f69424d2a1b669999

🚨 Report Summary

Severity Level Results
Contracts Critical
High
Medium
Low
Note
Total
2
1
0
10
39
52
Dependencies Critical
High
Medium
Low
Note
Total
0
0
0
0
0
0

For more details view the full report in OpenZeppelin Code Inspector

@alexcos20 alexcos20 added the Status: InProgress Work in progress, don't merge label Aug 7, 2024
@alexcos20 alexcos20 marked this pull request as draft August 7, 2024 14:37
@trentmc
Copy link
Member

trentmc commented Aug 7, 2024

Hey @alexcos20 I recommend to also update the README that describes the templates: https://github.com/oceanprotocol/contracts/tree/main/contracts/templates.

@trentmc
Copy link
Member

trentmc commented Aug 7, 2024

I recommend renaming it from "ERC20TemplateSapphire.sol" --> "ERC20Template4.sol".

Here's why:

First, calling it "ERC20TemplateSapphire.sol" is a misnomer. Since template 3 is also Sapphire-only. And it's tuned for Predictoor, of course.

Second, remember the problem with the name "ERC20TemplateEnterprise.sol": it was a template useful beyond enterprises too. However the "enterprise" portion always made people think it was just for enterprises.

Third: there's a good chance that we deploy >=1 new variant of template 3 for Predictoor. Eg for continuous-valued predictions, or other new features into the contract. Similarly, there's a good chance we deploy variants of the other templates too (including this new one). It starts to get messy to manage if the names are misleading. Hence why there's a simple name, plus a table to show what characteristics each template has.

If these weren't smart contracts, because of the issues above, we probably would have refactored the names into "ERC20Template1.sol", "ERC20Template2.sol", and "ERC20Template3.sol" long ago. But we only realized them when getting into template 3, and that's why it's already named ERC20Template3.sol". Let's keep this trend going: I recommend that this this new template, call it "ERC20Template4.sol".

Copy link
Member

@trentmc trentmc left a comment

Choose a reason for hiding this comment

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

See my two comments in the PR conversation: (a) update the table (b) change name to ERCTemplate4.sol.

@alexcos20
Copy link
Member Author

@alexcos20
Copy link
Member Author

alexcos20 commented Aug 7, 2024

@trentmc - renamed to ERC20Template4 in 80e682a

@alexcos20 alexcos20 marked this pull request as ready for review August 7, 2024 19:28
@alexcos20 alexcos20 requested a review from trentmc August 7, 2024 19:28
@alexcos20 alexcos20 removed the Status: InProgress Work in progress, don't merge label Aug 7, 2024
contracts/templates/README.md Outdated Show resolved Hide resolved
@alexcos20 alexcos20 requested a review from trentmc August 8, 2024 04:54
@alexcos20 alexcos20 changed the title Feature/template_sapphire ERC20Template4 - Confidential EVM Aug 13, 2024
@alexcos20 alexcos20 merged commit c49d299 into feature/accesslists Aug 13, 2024
9 checks passed
@alexcos20 alexcos20 deleted the feature/template_sapphire branch August 13, 2024 09:15
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