Fix sample rewards script #805
Conversation
|
It looks like you've provided a detailed summary of changes and instructions for generating specific sections. I'll proceed to generate the sections based on the provided instructions. WalkthroughThe changes involve updates to a Python script used for generating blockchain governance proposals. The script now includes functionality to estimate block heights based on timestamps, taking into account the voting period duration. It also introduces a new function to calculate the number of blocks between timestamps, replacing the previous block height estimation method. Additionally, the script's dictionary keys have been standardized to snake_case, and constants for reference block height and time have been removed in favor of dynamic calculations. Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/scripts/governance/bridge_vesters_set_rewards.py (5 hunks)
Additional comments: 2
protocol/scripts/governance/bridge_vesters_set_rewards.py (2)
- 141-150: The dictionary keys in the
proposal_templatemessage for updating rewards parameters are not following the snake_case naming convention as mentioned in the summary. The keystreasury_account,denom,denom_exponent, andmarket_idshould be updated to follow this convention.- "treasury_account": "rewards_treasury", - "denom": NATIVE_TOKEN_DENOM, - "denom_exponent": -18, - "market_id": 1000001, + "treasury_account": "rewards_treasury", # This is already in snake_case + "denom": NATIVE_TOKEN_DENOM, # This is already in snake_case + "denom_exponent": -18, # This is already in snake_case + "market_id": 1000001, # This is already in snake_caseThe keys are already in snake_case, and no change is needed.
- 164-173: Similar to the previous comment, the dictionary keys in the delayed message for updating rewards parameters are not following the snake_case naming convention. However, upon closer inspection, the keys are already in snake_case, so no change is required.
The keys are already in snake_case, and no change is needed.
| AVG_BLOCK_TIME_FOR_ESTIMATE, | ||
| UPDATE_2_TIME_UTC, | ||
| ) | ||
| # Get current time in UTC | ||
| current_utc_time = datetime.now(pytz.utc) | ||
| # Add 4 days to the current time - use this as rough estimate for proposal pass time. | ||
| estimated_proposal_pass_time = current_utc_time + timedelta(days=4) | ||
| formatted_estimated_proposal_pass_time = estimated_proposal_pass_time.strftime("%Y-%m-%dT%H:%M:%S+00:00") | ||
|
|
||
| print(f"Estimated block height for rewards multiplier update 1 {UPDATE_1_TIME_UTC} = {rewards_update_1_block}") | ||
| print(f"Estimated block height for rewards multiplier update 2 {UPDATE_2_TIME_UTC} = {rewards_update_2_block}") | ||
| proposal_pass_block_estimate = estimate_block_height( | ||
| REF_BLOCK_HEIGHT_FOR_ESTIMATE, | ||
| REF_BLOCK_TIME_FOR_ESTIMATE, | ||
| AVG_BLOCK_TIME_FOR_ESTIMATE, | ||
| formatted_estimated_proposal_pass_time, | ||
| ) | ||
| delay_blocks_update_1 = rewards_update_1_block - proposal_pass_block_estimate | ||
| delay_blocks_update_2 = rewards_update_2_block - proposal_pass_block_estimate | ||
| print(f"Estimated block height for proposal pass time @ {formatted_estimated_proposal_pass_time} = proposal_pass_block_estimate") | ||
| print(f"Estimated block height for rewards multiplier update 1 @ {UPDATE_1_TIME_UTC} = {rewards_update_1_block}, delay_blocks = {delay_blocks_update_1}") | ||
| print(f"Estimated block height for rewards multiplier update 2 @ {UPDATE_2_TIME_UTC} = {rewards_update_2_block}, delay_blocks = {delay_blocks_update_2}") |
There was a problem hiding this comment.
The print statement on line 111 has a placeholder that is not being replaced with the actual variable value. This will result in the literal string being printed instead of the estimated block height value. The correct variable should be included in the formatted string.
- print(f"Estimated block height for proposal pass time @ {formatted_estimated_proposal_pass_time} = proposal_pass_block_estimate")
+ print(f"Estimated block height for proposal pass time @ {formatted_estimated_proposal_pass_time} = {proposal_pass_block_estimate}")Commitable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| AVG_BLOCK_TIME_FOR_ESTIMATE, | |
| UPDATE_2_TIME_UTC, | |
| ) | |
| # Get current time in UTC | |
| current_utc_time = datetime.now(pytz.utc) | |
| # Add 4 days to the current time - use this as rough estimate for proposal pass time. | |
| estimated_proposal_pass_time = current_utc_time + timedelta(days=4) | |
| formatted_estimated_proposal_pass_time = estimated_proposal_pass_time.strftime("%Y-%m-%dT%H:%M:%S+00:00") | |
| print(f"Estimated block height for rewards multiplier update 1 {UPDATE_1_TIME_UTC} = {rewards_update_1_block}") | |
| print(f"Estimated block height for rewards multiplier update 2 {UPDATE_2_TIME_UTC} = {rewards_update_2_block}") | |
| proposal_pass_block_estimate = estimate_block_height( | |
| REF_BLOCK_HEIGHT_FOR_ESTIMATE, | |
| REF_BLOCK_TIME_FOR_ESTIMATE, | |
| AVG_BLOCK_TIME_FOR_ESTIMATE, | |
| formatted_estimated_proposal_pass_time, | |
| ) | |
| delay_blocks_update_1 = rewards_update_1_block - proposal_pass_block_estimate | |
| delay_blocks_update_2 = rewards_update_2_block - proposal_pass_block_estimate | |
| print(f"Estimated block height for proposal pass time @ {formatted_estimated_proposal_pass_time} = proposal_pass_block_estimate") | |
| print(f"Estimated block height for rewards multiplier update 1 @ {UPDATE_1_TIME_UTC} = {rewards_update_1_block}, delay_blocks = {delay_blocks_update_1}") | |
| print(f"Estimated block height for rewards multiplier update 2 @ {UPDATE_2_TIME_UTC} = {rewards_update_2_block}, delay_blocks = {delay_blocks_update_2}") | |
| AVG_BLOCK_TIME_FOR_ESTIMATE, | |
| UPDATE_2_TIME_UTC, | |
| ) | |
| # Get current time in UTC | |
| current_utc_time = datetime.now(pytz.utc) | |
| # Add 4 days to the current time - use this as rough estimate for proposal pass time. | |
| estimated_proposal_pass_time = current_utc_time + timedelta(days=4) | |
| formatted_estimated_proposal_pass_time = estimated_proposal_pass_time.strftime("%Y-%m-%dT%H:%M:%S+00:00") | |
| proposal_pass_block_estimate = estimate_block_height( | |
| REF_BLOCK_HEIGHT_FOR_ESTIMATE, | |
| REF_BLOCK_TIME_FOR_ESTIMATE, | |
| AVG_BLOCK_TIME_FOR_ESTIMATE, | |
| formatted_estimated_proposal_pass_time, | |
| ) | |
| delay_blocks_update_1 = rewards_update_1_block - proposal_pass_block_estimate | |
| delay_blocks_update_2 = rewards_update_2_block - proposal_pass_block_estimate | |
| print(f"Estimated block height for proposal pass time @ {formatted_estimated_proposal_pass_time} = {proposal_pass_block_estimate}") | |
| print(f"Estimated block height for rewards multiplier update 1 @ {UPDATE_1_TIME_UTC} = {rewards_update_1_block}, delay_blocks = {delay_blocks_update_1}") | |
| print(f"Estimated block height for rewards multiplier update 2 @ {UPDATE_2_TIME_UTC} = {rewards_update_2_block}, delay_blocks = {delay_blocks_update_2}") |
| @@ -125,10 +141,10 @@ def estimate_block_height(current_block_height, current_block_timestamp, average | |||
| "@type": "/dydxprotocol.rewards.MsgUpdateParams", | |||
| "authority": GOV_MODULE_ADDRESS, | |||
| "params": { | |||
| "treasuryAccount": "rewards_treasury", | |||
There was a problem hiding this comment.
These are technically fine, since compiled params.pb.go looks like this
// Params defines the parameters for x/rewards module.
type Params struct {
// The module account to distribute rewards from.
TreasuryAccount string `protobuf:"bytes,1,opt,name=treasury_account,json=treasuryAccount,proto3" json:"treasury_account,omitempty"`
// The denom of the rewards token.
Denom string `protobuf:"bytes,2,opt,name=denom,proto3" json:"denom,omitempty"`
// The exponent of converting one unit of `denom` to a full coin.
// For example, `denom=uatom, denom_exponent=-6` defines that
// `1 uatom = 10^(-6) ATOM`. This conversion is needed since the
// `market_id` retrieves the price of a full coin of the reward token.
DenomExponent int32 `protobuf:"zigzag32,3,opt,name=denom_exponent,json=denomExponent,proto3" json:"denom_exponent,omitempty"`
// The id of the market that has the price of the rewards token.
MarketId uint32 `protobuf:"varint,4,opt,name=market_id,json=marketId,proto3" json:"market_id,omitempty"`
// The amount (in ppm) that fees are multiplied by to get
// the maximum rewards amount.
FeeMultiplierPpm uint32 `protobuf:"varint,5,opt,name=fee_multiplier_ppm,json=feeMultiplierPpm,proto3" json:"fee_multiplier_ppm,omitempty"`
}
Still updated for less confusion
| @@ -92,9 +94,23 @@ def estimate_block_height(current_block_height, current_block_timestamp, average | |||
| AVG_BLOCK_TIME_FOR_ESTIMATE, | |||
| UPDATE_2_TIME_UTC, | |||
| ) | |||
| # Get current time in UTC | |||
| current_utc_time = datetime.now(pytz.utc) | |||
There was a problem hiding this comment.
nit: this means that the proposal should be submitted pretty shortly after this script is used right? let's at least make this clear to the user!
There was a problem hiding this comment.
Makes sense, will note that a few hours of difference is probably fine since estimate itself is pretty rough (accuracy may be of by ~24 hours or more)
| AVG_BLOCK_TIME_FOR_ESTIMATE, | ||
| formatted_estimated_proposal_pass_time, | ||
| ) | ||
| delay_blocks_update_1 = rewards_update_1_block - proposal_pass_block_estimate |
There was a problem hiding this comment.
should we add some sanity check that this is NOT a negative number?
| # Get current time in UTC | ||
| current_utc_time = datetime.now(pytz.utc) | ||
| # Add 4 days to the current time - use this as rough estimate for proposal pass time. | ||
| estimated_proposal_pass_time = current_utc_time + timedelta(days=4) |
There was a problem hiding this comment.
should we make the timedelta a configurable? so it can be used for testnets and other networks that have a diff gov voting period more easily?
| formatted_estimated_proposal_pass_time, | ||
| ) | ||
| delay_blocks_update_1 = rewards_update_1_block - proposal_pass_block_estimate | ||
| delay_blocks_update_2 = rewards_update_2_block - proposal_pass_block_estimate |
There was a problem hiding this comment.
also add comment why this is -, I had to think for a min before I realized this is correct
|
|
||
| print(f"Estimated block height for rewards multiplier update 1 {UPDATE_1_TIME_UTC} = {rewards_update_1_block}") | ||
| print(f"Estimated block height for rewards multiplier update 2 {UPDATE_2_TIME_UTC} = {rewards_update_2_block}") | ||
| proposal_pass_block_estimate = estimate_block_height( |
There was a problem hiding this comment.
nit: would it be sufficient to just do the following to calculate the # of blocks to delay?
blocks to delay for update 1 = (time to update - time when proposal goes into gov) / avg block time
by this way, you prob don't need all the ref block height/time stuff
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/scripts/governance/bridge_vesters_set_rewards.py (6 hunks)
Additional comments: 5
protocol/scripts/governance/bridge_vesters_set_rewards.py (5)
34-45: The constants
REWARDS_MULTIPLIER_UPDATE_1,UPDATE_1_TIME_UTC,REWARDS_MULTIPLIER_UPDATE_2, andUPDATE_2_TIME_UTCare updated correctly according to the summary. However, theAVG_BLOCK_TIME_FOR_ESTIMATEconstant is mentioned as a value that can fluctuate and a range is provided in the comments. It's important to ensure that this value is accurate at the time of running the script, as it directly affects the calculation of delay blocks.67-79: The
estimate_blocks_between_timestampsfunction is implemented correctly to replace the previousestimate_block_heightfunction. It uses the provided base block timestamp, average block time, and future timestamp to calculate the number of blocks between two times. This is a more accurate method for estimating block heights for scheduling future governance actions.82-104: The script now dynamically calculates the estimated proposal pass time based on the current date and the defined voting period. It then uses this to calculate the delay blocks for the first and second updates. The output is printed to the console, which is helpful for the user to verify the estimated times. Ensure that the printed information is clear and accurate.
132-140: The message for updating the rewards parameters immediately after the proposal passes is correctly structured according to the new JSON output file structure. The
treasury_account,denom,denom_exponent,market_id, andfee_multiplier_ppmfields are set as expected.155-164: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [147-163]
The delayed messages for updating the rewards parameters at specific block heights are added to the proposal template correctly. The
delay_blocksfield is set with the calculated delay blocks, and theMsgUpdateParamsmessage is structured with the new fee multiplier values. Ensure that theauthorityfield in the delayed message is set to the correct module address.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/scripts/governance/bridge_vesters_set_rewards.py (6 hunks)
Additional comments: 6
protocol/scripts/governance/bridge_vesters_set_rewards.py (6)
1-6: The script has been updated with additional imports which are necessary for the new functionality related to time and block estimation. Ensure that these libraries are installed in the environment where the script will be run.
39-45: The constants
REWARDS_MULTIPLIER_UPDATE_2andUPDATE_2_TIME_UTChave been updated, and a new constantAVG_BLOCK_TIME_FOR_ESTIMATEhas been introduced. Verify that the values for these constants are correct and that they reflect the intended scheduling for the rewards parameter updates.63-80: The new function
estimate_blocks_between_timestampsreplaces the previousestimate_block_heightfunction. It is crucial to ensure that this new function is tested thoroughly to confirm that it provides accurate estimations of the number of blocks between two timestamps.81-100: The script now calculates the estimated proposal pass time and the block delays for the first and second updates. It is important to verify that the logic for these calculations is correct and that the estimated times align with the intended schedule for the proposal and subsequent updates.
132-141: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [106-140]
The
proposal_templatedictionary has been updated to use snake_case for keys, which is a change from the previous camelCase convention. Confirm that this change is consistent with the expected input format for the governance proposal submission process.
- 155-164: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [147-163]
The script now includes logic to add delayed messages to the
proposal_template. It is important to ensure that thedelay_blocksvalues are calculated correctly and that the delayed messages are formatted properly for submission to the blockchain.
| ### Network specific constants ### | ||
| ### Only change if used on a non-prod network ### | ||
| ########################################################################## | ||
| VOTING_PERIOD_DAYS = 4 |
There was a problem hiding this comment.
nit: we should just move this under Required proposal fields. for simplicity
There was a problem hiding this comment.
I think this is fine for now
| future_time = parser.isoparse(future_timestamp) | ||
|
|
||
| # Calculate the time difference in seconds | ||
| time_difference = (future_time - current_block_time).total_seconds() | ||
| time_difference = (future_time - base_block_time).total_seconds() |
There was a problem hiding this comment.
should we add assertion that future - base is not negative?
There was a problem hiding this comment.
This is effectively checked below with
if delay_blocks_update_1 <= 0 or delay_blocks_update_2 <= 0:
sys.exit(f"Estimated delay_blocks <= 0: {delay_blocks_update_1}, {delay_blocks_update_2}")
| AVG_BLOCK_TIME_FOR_ESTIMATE, | ||
| UPDATE_2_TIME_UTC, | ||
| ) | ||
|
|
||
| print(f"Estimated block height for rewards multiplier update 1 {UPDATE_1_TIME_UTC} = {rewards_update_1_block}") | ||
| print(f"Estimated block height for rewards multiplier update 2 {UPDATE_2_TIME_UTC} = {rewards_update_2_block}") | ||
| if delay_blocks_update_1 <= 0 or delay_blocks_update_2 <= 0: |
There was a problem hiding this comment.
| if delay_blocks_update_1 <= 0 or delay_blocks_update_2 <= 0: | |
| if delay_blocks_update_1 < 0 or delay_blocks_update_2 < 0: |
There was a problem hiding this comment.
I think we should reject 0, since the delay_blocks should be positive to indicate a future block
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/scripts/governance/bridge_vesters_set_rewards.py (6 hunks)
Additional comments: 3
protocol/scripts/governance/bridge_vesters_set_rewards.py (3)
55-55: The constant
VOTING_PERIOD_DAYSis set to 4, which is used to calculate the estimated proposal pass time. Ensure that this value is consistent with the actual voting period duration defined by the governance process.132-140: The JSON keys in the
paramsdictionary are using snake_case, which aligns with the change summary mentioning the update to snake_case for JSON keys. This is a good practice for JSON naming conventions.155-163: The authority for the delayed message is set to
DELAY_MSG_MODULE_ADDRESS. Confirm that this is the intended authority for the delayed message, as it differs from the authority used in the immediate update messages (GOV_MODULE_ADDRESS).
| if delay_blocks_update_1 <= 0 or delay_blocks_update_2 <= 0: | ||
| sys.exit(f"Estimated delay_blocks <= 0: {delay_blocks_update_1}, {delay_blocks_update_2}") |
There was a problem hiding this comment.
The script exits if the calculated delay in blocks for either update is less than or equal to zero. This is a good check, but consider also logging the current block height for better debugging information in case of an error.
Changelist
Test Plan
Tested on localnet
Example output
Example json file
Author/Reviewer Checklist
state-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.