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

Removed Mint method #130

Merged
merged 17 commits into from Jun 2, 2021
Merged

Removed Mint method #130

merged 17 commits into from Jun 2, 2021

Conversation

adi44
Copy link
Contributor

@adi44 adi44 commented May 19, 2021

In the earlier version of smart contracts, the stake manager was minting the block reward amount every time. Which is quite a different approach from what we have in the whitepaper. In this PR, I have removed the mint functionality since we are going to send native tokens.

What did I do in this PR?

I removed the require(schellingCoin.mint(amount)) from the stakeManager.sol. In the Post Deployment setup, instead of minting tokens to stakeManager, I am transferring the tokens from the deployer address to the stakemanager.

The total supply of the schelling Coin is 1 Billion, in order to keep this much amount, I have implemented the formulas, which leads the stake Manager Contract to have 600 Million tokens and Deployer address 400 Million.

Now Since, we don't need the stakeManager to be the Minter, I have removed the Constructor Parameter that we were using during the test deployment as well as from the contract constructor(address minter).

How PostDeploymentSetup works now?

Post Deployment setup, checks if it is mainnet or not, if it is not mainnet, it will calculate the mintableSupply which is 600 MIllion. and sent it to the stakemanager. If deployer is using the old schelling Coin address, the deployer will be minted the tokens, that will be equal to the (Intial Supply - Deployers Current balance) so it don't go out of supply.

Fixes #108

@adi44 adi44 requested a review from dev1644 May 19, 2021 13:51
@adi44 adi44 changed the title Mint method Removed Mint method May 19, 2021
@adi44 adi44 requested review from SamAg19 and SkandaBhat May 20, 2021 11:15
// if previous instances of Schelling Coin is reused again and again,
// then initial balance will get depleted, thus intial tokens minting is needed,
// each time Schelling Coin instance is reused
const initialSupply = await schellingCoin.INITIAL_SUPPLY();
await schellingCoin.mint(signers[0].address, initialSupply);
await schellingCoin.mint(stakeManagerAddress, initialSupply);
Copy link
Contributor

Choose a reason for hiding this comment

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

await schellingCoin.mint(stakeManagerAddress, initialSupply);

We can remove this from here & use it after the if condition. @adi44

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

@adi44 adi44 requested a review from dev1644 May 22, 2021 07:09
migrations/src/postDeploymentSetup.js Outdated Show resolved Hide resolved
@adi44 adi44 requested a review from dev1644 May 25, 2021 04:56
adi44 added 2 commits May 25, 2021 10:35
removed mint method

Added Mint method after if condition

solved linting issues

Removed duplicate mint functionality

removed duplicate mint functionality
dev1644
dev1644 previously approved these changes May 25, 2021
await schellingCoin.addMinter(signers[0].address);

if (SCHELLING_COIN_ADDRESS !== '') {
const { StakeManager: oldStakeManagerAddress } = await readOldDeploymentFile();

// if previous instances of Schelling Coin is reused again and again,
// then initial balance will get depleted, thus intial tokens minting is needed,
// each time Schelling Coin instance is reused
const initialSupply = await schellingCoin.INITIAL_SUPPLY();
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove this declaration, it is redundant to line no.46

adi44 and others added 2 commits May 31, 2021 10:47
Uploaded using Different PR specifically for Matic delpoyment
@adi44 adi44 requested a review from 0xcuriousapple May 31, 2021 06:33
await schellingCoin.addMinter(signers[0].address);
const initialSupply = await schellingCoin.INITIAL_SUPPLY();
const stakeManagerSupply = (BigNumber.from(10).pow(BigNumber.from(26))).mul(BigNumber.from(6));
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename this to MintableSupply

Copy link
Contributor

Choose a reason for hiding this comment

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

Please rename this to mintableSupply as we discussed.

await schellingCoin.removeMinter(signers[0].address);
}

// Remove previous instance of StakeManager contract & Deployer address from Minter
await schellingCoin.mint(stakeManagerAddress, initialSupply);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey
First we have to only mint mintableSupply to stakaManager, not initial supply
See
If shelling coin get deployed, deployer get complete Initial Supply minted.
Now again you mintinng here on top of it doesn't make sense.
It should be transfer

So impl as per me would


 if (SCHELLING_COIN_ADDRESS !== '') { 
 Mint Intial Supply to Signers[0]
 } 
Transfer Mintable Supply from Signers[0] to StakeManager

This would work in both cases

@@ -44,23 +44,25 @@ module.exports = async () => {
// Only transfer tokens in testnets
if (NETWORK !== 'mainnet') {
// Add new instance of StakeManager contract & Deployer address as Minter
await schellingCoin.addMinter(stakeManagerAddress);

await schellingCoin.addMinter(signers[0].address);
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is not needed IMO
If deployed new, signers[0] is minter only by constructor
If reusing we are adding him as minter at line
https://github.com/razor-network/contracts/pull/130/files#diff-268ac3aa71a022d9f10b6c5603857967f9f237b5f58ff6a1569ea63a2f2141c8R56

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abhishekvispute I have made changes as per the discussion. I have resolved issue #141 too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @adi44
Please remove grant role as well from shelling coin constructor as we discussed.
And also Please update PR title and desc for same

await schellingCoin.addMinter(signers[0].address);
const initialSupply = await schellingCoin.INITIAL_SUPPLY();
const stakeManagerSupply = (BigNumber.from(10).pow(BigNumber.from(26))).mul(BigNumber.from(6));
Copy link
Contributor

Choose a reason for hiding this comment

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

Please rename this to mintableSupply as we discussed.

// Remove previous instance of StakeManager contract & Deployer address from Minter
await schellingCoin.removeMinter(oldStakeManagerAddress);
await schellingCoin.addMinter(signers[0].address);
if (schellingCoin.balanceOf(signers[0].address) === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey Why do you think this condn is needed
As per me

 await schellingCoin.mint(signers[0].address, initialSupply - schellingCoin.balanceOf(signers[0].address));

Is all we need, if signers[0] balance is 0, mint Initial Supply - 0 == Intial supply
Thats expected behav

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah! Agreed!

Copy link
Contributor

@0xcuriousapple 0xcuriousapple left a comment

Choose a reason for hiding this comment

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

Hey @adi44
Please add what you changed and why in PR desc
Also add ref to new issue
All else LGTM

@adi44 adi44 requested a review from 0xcuriousapple June 1, 2021 04:46
0xcuriousapple
0xcuriousapple previously approved these changes Jun 1, 2021
SkandaBhat
SkandaBhat previously approved these changes Jun 2, 2021
@SkandaBhat SkandaBhat dismissed stale reviews from 0xcuriousapple and themself via 83a0eff June 2, 2021 05:48
@0xcuriousapple
Copy link
Contributor

Hi @SkandaBhat
Please do squash and merge.
As there are lot of redundant commits.

Copy link
Contributor

@0xcuriousapple 0xcuriousapple left a comment

Choose a reason for hiding this comment

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

LGTM

@SkandaBhat SkandaBhat merged commit 6b9c1c1 into master Jun 2, 2021
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.

Remove Mint and transfer rewards from StakeManager
4 participants