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

Newlevel safetoken #10

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Newlevel safetoken #10

wants to merge 2 commits into from

Conversation

JithinKS97
Copy link
Owner

@JithinKS97 JithinKS97 commented Jan 10, 2023

Added the following changes on top of OpenZeppelin#193

  1. moved files
  2. upgraded solidity levels
  3. upgraded contracts
  4. corrected the tests
  5. changed bytecode decompiler
  6. added review comment change
  7. added images

@JithinKS97 JithinKS97 force-pushed the newlevel-safetoken branch 2 times, most recently from caa1895 to 5dfb2c0 Compare January 10, 2023 07:56


Things that might help:
* An [EVM bytecode decompiler](https://ropsten.etherscan.io/bytecode-decompiler?a=)
Copy link

Choose a reason for hiding this comment

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

Ropsten is deprecated, maybe is worth changing it for some other tool ? or the same from etherscan but in another chain

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes sure, I'll look at that.

@@ -147,6 +147,11 @@
"websites": [
"https://www.linkedin.com/in/kstasi/"
]
},
"AshiqAmien": {
"name": "Ashiq Amien",
Copy link

Choose a reason for hiding this comment

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

Are all these meant to be arrays instead of single key values ?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Oh thanks for the catch, changing now

"revealCode": true,
"deployParams": [],
"deployFunds": 0,
"deployId": "29",
Copy link

Choose a reason for hiding this comment

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

This conflicts with OpenZeppelin#546

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes, we can merge this once 546 is merged. I will rebase it with master once 546 is merged.

Copy link

Choose a reason for hiding this comment

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

Yes but we need to change deployId to 30

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import '@openzeppelin/contracts/token/ERC20/ERC20.sol';
Copy link

Choose a reason for hiding this comment

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

Shouldn't this import be similar to the one of SafeTokenBackdoor ?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes both are basically, same, we have
import 'openzeppelin-contracts-08/token/ERC20/ERC20.sol'; and
import '@openzeppelin/contracts/token/ERC20/ERC20.sol';

To keep things consistent, we will keep it the same.

@JithinKS97 JithinKS97 force-pushed the newlevel-safetoken branch 4 times, most recently from 4c8e16c to ef69a3a Compare January 10, 2023 11:37
moved files

upgraded solidity levels

upgraded contracts

corrected the tests

changed bytecode decompiler

added review comment changes

added images
@JithinKS97 JithinKS97 force-pushed the master branch 12 times, most recently from edaa0f7 to 5b14709 Compare February 3, 2023 05:37
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.

3 participants