[Mainnet] Update StandardToken for uint256 support#32
Merged
StratisIain merged 8 commits intostratisproject:masterfrom Dec 29, 2021
Merged
[Mainnet] Update StandardToken for uint256 support#32StratisIain merged 8 commits intostratisproject:masterfrom
StratisIain merged 8 commits intostratisproject:masterfrom
Conversation
mrtpain
reviewed
Jan 15, 2021
| /// <param name="name">The name of the token.</param> | ||
| /// <param name="symbol">The symbol used to identify the token.</param> | ||
| public StandardToken(ISmartContractState smartContractState, ulong totalSupply, string name, string symbol) | ||
| public StandardToken(ISmartContractState smartContractState, Amount totalSupply, string name, string symbol) |
Collaborator
There was a problem hiding this comment.
Can Amount be changed to UInt256? How would UInt128 be used? If possible it would be nice to use those types explicitly as UInt256 or UInt128.
Also, while we're here, can we add the Decimals property from this PR #17
Contributor
There was a problem hiding this comment.
I think it is better to use UInt256 explicitly in here otherwise it is confusing.
Contributor
There was a problem hiding this comment.
Now decimals update pr duplicated :) I want to push these changes first then can you apply your changes ? @mrtpain
Contributor
There was a problem hiding this comment.
@quantumagi add decimals parameter to constructor and set it to property. Decimals methods should to be updated as Property getters/setters. @quantumagi
Collaborator
There was a problem hiding this comment.
@YakupIpek @quantumagi - #17 is ready for review and merging, it's probably easiest/best to review and merge that into master, then update this branch with those changes and update the README included in that PR with the latest bytecode and hash after those changes are merged back into this branch.
Contributor
|
This pr can be merged after Amount alias is removed. @quantumagi |
Contributor
|
Btw it would be better to update testnet files first then we can create mainnet after being sure it is fine. Currently these are mainnet files. |
Contributor
Author
|
We should not release this code until a sufficient number of nodes have upgraded to PR 365: stratisproject/StratisFullNode#365. |
YakupIpek
reviewed
Jan 28, 2021
Contributor
Author
|
@YakupIpek , have you completed your review of this PR? |
Collaborator
|
Accidentally added comments to Testnet, should've been here. I approved already but would be good to update |
mrtpain
reviewed
Feb 10, 2021
Collaborator
|
Pinging this and the related #33 PR for Testnet. Is EC Recover still a work in progress? Possibly the reason to keep these open rather than merging yet. |
Collaborator
|
Checked this against Testnet, there are some differences that can be copied/pasted over from Testnet whitelisted contract.
Only those two changes needed + compiling for new hash and bytecode. Might be worth adding a README to the directory including the hash and bytecode so developers know what it should be to compare with what they get when compiling. After that, this will be ready for a Mainnet whitelisting vote. |
Contributor
Author
|
I see some issues with the readme idea: (1) People verifying the code but not checking if the code corresponds to the hash. I.e. its better if people generate the hash for themselves. |
This was referenced Dec 24, 2021
Contributor
|
Whitelisted |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.