-
Notifications
You must be signed in to change notification settings - Fork 454
Add missing configurable config params #8854
Add missing configurable config params #8854
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## release/6.0.0 #8854 +/- ##
=================================================
+ Coverage 83.57% 83.59% +0.01%
=================================================
Files 601 601
Lines 22486 22521 +35
Branches 3322 3324 +2
=================================================
+ Hits 18793 18826 +33
- Misses 3693 3695 +2
|
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.
Very good job! I tried to check as carefully as possible, caught just few minor inconsistencies.
export const defaultConfig = { | ||
minFeePerByte: 1000, | ||
maxBlockHeightZeroFeePerByte: 0, | ||
minFeePerByte: MIN_FEE_PER_BYTE, |
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 am not 100% sure those values are intended as part of default configuration, perhaps we should check this.
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 was following the specs. Let me know if they are outdated.
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.
Those are the mainchain values. Mainchain configuration is set in LIP 63. For sidechains built with SDK, I think the values should be given dynamically at creation time, same as TOKEN_ID_FEE
. Using current status, we define minFee to a default value without knowing which token it corresponds to, whic is a bit funny.
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.
Those are just some default values because we need some meaningful defaults, which users are supposed to override with their custom config?
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.
@gkoumout you mean to force user to set this value in the config for the sidechain?
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.
lockingPeriodSelfStaking: number; | ||
reportMisbehaviorReward: bigint; | ||
reportMisbehaviorLimitBanned: number; | ||
weightScaleFactor: bigint; | ||
} | ||
|
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.
Comment about line 33: round length is not configurable anymore..
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.
Round Length is no longer a directly configurable param, but a derived param. It is calculated based on number of configured validators.
So it is still part of the config.
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.
ok sounds good, just wanted to make sure.
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.
Overall LGTM, one minor problem i see
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.
Some minor leftover comments.
export const defaultConfig = { | ||
minFeePerByte: 1000, | ||
maxBlockHeightZeroFeePerByte: 0, | ||
minFeePerByte: MIN_FEE_PER_BYTE, |
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.
Those are the mainchain values. Mainchain configuration is set in LIP 63. For sidechains built with SDK, I think the values should be given dynamically at creation time, same as TOKEN_ID_FEE
. Using current status, we define minFee to a default value without knowing which token it corresponds to, whic is a bit funny.
lockingPeriodSelfStaking: number; | ||
reportMisbehaviorReward: bigint; | ||
reportMisbehaviorLimitBanned: number; | ||
weightScaleFactor: bigint; | ||
} | ||
|
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.
ok sounds good, just wanted to make sure.
export const defaultConfig = { | ||
minFeePerByte: 1000, | ||
maxBlockHeightZeroFeePerByte: 0, | ||
minFeePerByte: MIN_FEE_PER_BYTE, |
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.
@gkoumout you mean to force user to set this value in the config for the sidechain?
What was the problem?
This PR resolves #8753
How was it solved?
Replaced config literals with constants:
MAX_TRANSACTIONS_SIZE
BFT_BATCH_SIZE
FACTOR_MINIMUM_REWARD_ACTIVE_VALIDATORS
MIN_FEE_PER_BYTE
MAX_BLOCK_HEIGHT_ZERO_FEE_PER_BYTE
FACTOR_SELF_STAKES
FAIL_SAFE_MISSED_BLOCKS
FAIL_SAFE_INACTIVE_WINDOW
MAX_BFT_WEIGHT_CAP
USE_INVALID_BLS_KEY
Added a default value in
defaultConfig
and updated code not to directly import the constant, but get it from config:NUMBER_ACTIVE_VALIDATORS
NUMBER_STANDBY_VALIDATORS
MAX_LENGTH_NAME
MAX_NUMBER_PENDING_UNLOCKS
BASE_STAKE_AMOUNT
LOCKING_PERIOD_SELF_STAKING
WEIGHT_SCALE_FACTOR
PUNISHMENT_PERIOD
PUNISHMENT_WINDOW_STAKING
PUNISHMENT_WINDOW_SELF_STAKING
LOCKING_PERIOD_SELF_STAKING
Added as default value in
defaultConfig
, added config property, and updated code not to directly import the constant, but get it from config:LOCKING_PERIOD_STAKING
REPORT_MISBEHAVIOR_REWARD
REPORT_MISBEHAVIOR_LIMIT_BANNED
MAX_NUMBER_OF_SIGNATURES
Added comment about different name:
MAX_COMMISSION_INCREASE_RATE
Renamed constant and added a comment about different name:
MAX_LENGTH_REVEALS
Renamed constant to align with LIP and property name:
USER_SUBSTORE_INITIALIZATION_FEE
=>USER_ACCOUNT_INITIALIZATION_FEE
ESCROW_SUBSTORE_INITIALIZATION_FEE
=>ESCROW_ACCOUNT_INITIALIZATION_FEE
TRANSACTION_MAX_PARAMS_SIZE
=>MAX_PARAMS_SIZE
Updated constant value to match LIP:
MAX_ASSET_DATA_SIZE_BYTES
Updated PoS util functions to get param value from config as arguments, instead of importing constants.
isCurrentlyPunished()
getWaitTime()
getPunishTime()
hasWaited()
isPunished()
getPunishmentPeriod()
BONUS: Changed
factorSelfStakes
tonumber
and removed all castings of it toBigInt
😎How was it tested?
All tests pass 👌🏻
Example app runs 🏃🏻