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.

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

Lower Proposal Threshold to 25K #189

Merged
merged 1 commit into from
Mar 29, 2022

Conversation

ratankaliani
Copy link
Contributor

@ratankaliani ratankaliani commented Feb 23, 2022

This PR upgrades the GovernorBravoDelegate contract to reflect the reduction of the minimum "setable" proposal threshold to 1K. Additionally, it adds simulations to test reducing the proposal threshold to 25K and updates the ABI to match Compound's current ABI state (cc @arr00) & updates the address for the GovernorBravoDelegate contract.

  • Add new GovernorBravoDelegate.sol and
  • Update ABI to the current mainnet config
  • Adds Proposal Simulations to test the proposal

@ratankaliani ratankaliani force-pushed the master branch 2 times, most recently from 9e0ecee to 334449c Compare March 10, 2022 22:40
@ratankaliani ratankaliani changed the title Lower Proposal Thresholds v2 Lower Proposal Threshold to 25K Mar 20, 2022
"address": "0x7b5e3521a049C8fF88e6349f33044c6Cc33c113c",
"contract": "Comptroller",
"description": "Standard Comptroller Impl"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Other changes lgtm but did you have to modify this whole file? 😬

Copy link
Contributor Author

@ratankaliani ratankaliani Mar 21, 2022

Choose a reason for hiding this comment

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

Cleaned mainnet.json to match the repo's structure of the file, however reverting mainnet-abi.json would bring it out of sync with the current state of protocol on-chain (cc @arr00).

Should I revert mainnet-abi.json to match the current state of master?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, thank you! I wonder what actually changed in mainnet-abi.json besides the new abi?

Copy link
Contributor Author

@ratankaliani ratankaliani Mar 22, 2022

Choose a reason for hiding this comment

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

IIRC the existing mainnet-abi.json does not handle GovernorBravo's structure correctly with respect to .scen files, which is why @arr00 recommended using this ABI instead of the existing ABI.

I'm not as familiar with the specific changes to the ABI, but can dive deeper/remove it from this PR if you think we should include what changed in the ABI within this PR. I was running into significant issues testing with the existing ABI, which is why I included it in this PR, so that future devs wouldn't experience similar issues with the ABI. LMK what you think is best!

@ratankaliani
Copy link
Contributor Author

Anything else left on the checklist before merging?

@jflatow jflatow merged commit 3affca8 into compound-finance:master Mar 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants