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.

Sign up for GitHub

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

Fix sample rewards script #805

Merged
merged 4 commits into from
Nov 24, 2023
+44 −37
Merged

Fix sample rewards script #805

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8b0d92d
use estimated proposal pass time to calculate delay_blocks
teddyding Nov 24, 2023
77d9b0f
fix comments
teddyding Nov 24, 2023
be6480b
update default time
teddyding Nov 24, 2023
386a4f6
address comments
teddyding Nov 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 26 additions & 10 deletions protocol/scripts/governance/bridge_vesters_set_rewards.py
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import json
from dateutil import parser
from datetime import datetime, timedelta
import pytz

NINE_ZEROS="000000000"

@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

# 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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

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(
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea!

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
Copy link
Contributor

Choose a reason for hiding this comment

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

should we add some sanity check that this is NOT a negative number?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

delay_blocks_update_2 = rewards_update_2_block - proposal_pass_block_estimate
Copy link
Contributor

Choose a reason for hiding this comment

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

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 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}")
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Suggested change
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}")


proposal_template = {
"title": TITLE,
@@ -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",
Copy link
Contributor Author

@teddyding teddyding Nov 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

"treasury_account": "rewards_treasury",
"denom": NATIVE_TOKEN_DENOM,
"denomExponent": -18,
"marketId": 1000001,
"denom_exponent": -18,
"market_id": 1000001,
"fee_multiplier_ppm": REWARDS_MULTIPLIER
}
},
@@ -137,8 +153,8 @@ def estimate_block_height(current_block_height, current_block_timestamp, average

# Add delayed messages
for delayed_block_number, new_fee_multiplier in [
(rewards_update_1_block, REWARDS_MULTIPLIER_UPDATE_1),
(rewards_update_2_block, REWARDS_MULTIPLIER_UPDATE_2),
(delay_blocks_update_1, REWARDS_MULTIPLIER_UPDATE_1),
(delay_blocks_update_2, REWARDS_MULTIPLIER_UPDATE_2),
]:
proposal_template["messages"].append(
{
@@ -148,10 +164,10 @@ def estimate_block_height(current_block_height, current_block_timestamp, average
"@type": "/dydxprotocol.rewards.MsgUpdateParams",
"authority": DELAY_MSG_MODULE_ADDRESS,
"params": {
"treasuryAccount": "rewards_treasury",
"treasury_account": "rewards_treasury",
"denom": NATIVE_TOKEN_DENOM,
"denomExponent": -18,
"marketId": 1000001,
"denom_exponent": -18,
"market_id": 1000001,
"fee_multiplier_ppm": new_fee_multiplier
}
},