-
Notifications
You must be signed in to change notification settings - Fork 68
Record total minted API v2 #1769
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
base: dev-upgrade
Are you sure you want to change the base?
Conversation
| slotMintedRecordOnsetBlock = common.HexToHash("0000000000000000000000000000000000000000000000000000000000000002") | ||
| slotMintedRecordPostTotalMintedBase, _ = new(big.Int).SetString("0100000000000000000000000000000000000000000000000000000000000000", 16) | ||
| slotMintedRecordPostTotalBurnedBase, _ = new(big.Int).SetString("0200000000000000000000000000000000000000000000000000000000000000", 16) | ||
| slotMintedRecordPostRewardBlockBase, _ = new(big.Int).SetString("0300000000000000000000000000000000000000000000000000000000000000", 16) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add 0x prefix in string ? I think it's more clear
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
common.HexToHash can. done.
new(big.Int).SetString cannot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/ethereum/go-ethereum/blob/master/crypto/secp256k1/curve.go#L267-L271:
func init() {
// See SEC 2 section 2.7.1
// curve parameters taken from:
// http://www.secg.org/sec2-v2.pdf
theCurve.P, _ = new(big.Int).SetString("0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEFFFFFC2F", 0)
theCurve.N, _ = new(big.Int).SetString("0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141", 0)
theCurve.B, _ = new(big.Int).SetString("0x0000000000000000000000000000000000000000000000000000000000000007", 0)
theCurve.Gx, _ = new(big.Int).SetString("0x79BE667EF9DCBBAC55A06295CE870B07029BFCDB2DCE28D959F2815B16F81798", 0)
theCurve.Gy, _ = new(big.Int).SetString("0x483ADA7726A3C4655DA4FBFC0E1108A8FD17B448A68554199C47D08FFB10D4B8", 0)
theCurve.BitSize = 256
}
eth/hooks/engine_v2_hooks.go
Outdated
| // calculate the tokens before TIPUpgradeReward and set to totalMinted | ||
| // for now no-do | ||
| totalMinted := big.NewInt(0) | ||
| totalBurned := big.NewInt(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new(big.Int) is better than big.NewInt(0)
eth/hooks/engine_v2_hooks.go
Outdated
| for epochNumIter > 0 { | ||
| totalMinted = state.GetPostTotalMinted(stateBlock, epochNumIter-1).Big() | ||
| totalBurned = state.GetPostTotalBurned(stateBlock, epochNumIter-1).Big() | ||
| if totalMinted.BitLen() != 0 || totalBurned.BitLen() != 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should use Sign() to compare with 0
internal/ethapi/api.go
Outdated
| func (s *BlockChainAPI) GetTokenSupply(ctx context.Context, epochNr rpc.EpochNumber) (*tokenSupply, error) { | ||
| engine, ok := s.b.Engine().(*XDPoS.XDPoS) | ||
| if !ok { | ||
| return nil, errors.New("Undefined XDPoS consensus engine") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error string should started with lowercase, use undefined instead of Undefined
| call: 'eth_getCurrentTotalMinted', | ||
| params: 0, | ||
| name: 'getTokenSupply', | ||
| call: 'eth_getTokenSupply', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name is not precise, which token ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
XDC token. I think token is enough, no need xdc token.
core/state/statedb_utils.go
Outdated
| func GetPostTotalMinted(statedb *StateDB, epoch uint64) common.Hash { | ||
| hash := common.BigToHash(new(big.Int).Add(slotMintedRecordPostTotalMintedBase, new(big.Int).SetUint64(epoch))) | ||
| v := statedb.GetState(common.MintedRecordAddressBinary, hash) | ||
| return v |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not use return statedb.GetState(common.MintedRecordAddressBinary, hash)
eth/hooks/engine_v2_hooks.go
Outdated
| } | ||
| totalMinted.Add(totalMinted, rewardSum) | ||
| bigPower256 := new(big.Int).Lsh(big.NewInt(1), 256) | ||
| bigMaxU256 := new(big.Int).Sub(bigPower256, big.NewInt(1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use MaxUint256 in abi package, or define a package level variable:
MaxUint256 = new(big.Int).Sub(new(big.Int).Lsh(common.Big1, 256), common.Big1)No need to create bigPower256 and bigMaxU256 each time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or use math.MaxBig256
internal/ethapi/api.go
Outdated
| statedb, header, err := s.b.StateAndHeaderByNumber(ctx, rpc.LatestBlockNumber) | ||
| nonce := statedb.GetNonce(common.MintedRecordAddressBinary) | ||
| if nonce == 0 { | ||
| return nil, errors.New("MintedRecordAddress is not initialized due to Reward Upgrade is not applied") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error strings should not be capitalized
eth/hooks/engine_v2_hooks.go
Outdated
| // if overflow, set to maxU256 and log a warning | ||
| if totalMinted.Cmp(bigMaxU256) >= 0 { | ||
| if totalMinted.Cmp(bigMaxU256) > 0 { | ||
| totalMinted.Set(bigMaxU256) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think totalMinted and totalBurned will be greater than bigMaxU256 one day.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that will be very long time in the future. this PR can skip that.
internal/ethapi/api.go
Outdated
| preEpochMinted := new(big.Int).Mul(new(big.Int).SetUint64(config.Reward), new(big.Int).SetUint64(params.Ether)) | ||
| onsetEpochMinus := onsetEpoch | ||
| if onsetEpochMinus > 0 { | ||
| onsetEpochMinus -= 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onsetEpochMinus--
internal/ethapi/api.go
Outdated
| if onsetEpochMinus > 0 { | ||
| onsetEpochMinus -= 1 | ||
| } else { | ||
| log.Warn("onsetEpoch is 0 which could not happen", epochNum) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log message string should be capitalized
| call: 'eth_getTokenSupply', | ||
| params: 1, | ||
| }), | ||
| ], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we define command in gethclient ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it is useful, then we shall do it
internal/ethapi/api.go
Outdated
| PostUpgradeTotalMinted *hexutil.Big `json:"postUpgradeTotalMinted"` | ||
| PreUpgradeTotalMinted *hexutil.Big `json:"preUpgradeTotalMinted"` | ||
| PostUpgradeTotalBurned *hexutil.Big `json:"postUpgradeTotalBurned"` | ||
| TotalMinted *hexutil.Big `json:"totalMinted"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you
- remove
Totalfor all variables as it always represents total - create like sub object like
- rewardV2.minted
- rewardV2.burned
- rewardV1.minted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PostUpgrade and PreUpgrade. I think reward v1 v2 is more easier understand
Proposed changes
Record total minted API with additional parameter
epoch number. Record both minted and burned. Renamed to getTokenSupply.Types of changes
What types of changes does your code introduce to XDC network?
Put an
✅in the boxes that applyImpacted Components
Which part of the codebase this PR will touch base on,
Put an
✅in the boxes that applyChecklist
Put an
✅in the boxes once you have confirmed below actions (or provide reasons on not doing so) that