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

[Consolidating] PR#451: Incorporate PR#447 Governance vesting cancelling & PR#448 Staking refactoring #451

Conversation

cwsnt
Copy link
Contributor

@cwsnt cwsnt commented Sep 14, 2022

No description provided.

cwsnt and others added 30 commits July 11, 2022 07:58
add team vesting validation for governance withdrawal
adjust four year vesting
Adjust script for governance withdraw vesting
revert getPriorUserStakeByDate for backward compatibility
improve doc & natspec based on review
add script for registering vesting detail in registry
remove unused event
improve storage variable naming for vesting creation type
@cwsnt cwsnt changed the title [Consolidation] Staking Rewards PR#448 & Fix Governance Withdraw Vesting PR #448 Draft - [Consolidation] Staking Rewards PR#448 & Fix Governance Withdraw Vesting PR #448 Sep 14, 2022
@tjcloa tjcloa changed the base branch from development to refactoring/staking-contract-eip170 September 14, 2022 12:38
@cwsnt cwsnt changed the title Draft - [Consolidation] Staking Rewards PR#448 & Fix Governance Withdraw Vesting PR #448 [Consolidation] Staking Rewards PR#448 & Fix Governance Withdraw Vesting PR #448 Sep 16, 2022
Copy link
Contributor

@tjcloa tjcloa left a comment

Choose a reason for hiding this comment

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

requires a merge from the target branch: (staking-contract-eip170) and conflicts resolution.

contracts/governance/Staking/Staking.sol Outdated Show resolved Hide resolved
contracts/governance/Vesting/VestingRegistryLogic.sol Outdated Show resolved Hide resolved
scripts/sip/sip_interaction.py Outdated Show resolved Hide resolved
Copy link
Contributor

@tjcloa tjcloa left a comment

Choose a reason for hiding this comment

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

requires a merge from the target branch: (staking-contract-eip170) and conflicts resolution.

@cwsnt cwsnt requested a review from tjcloa September 19, 2022 09:48
Copy link
Contributor

@tjcloa tjcloa left a comment

Choose a reason for hiding this comment

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

pls see unresolved issues - clarified all questions hopefully).

scripts/sip/sip_interaction.py Outdated Show resolved Hide resolved
contracts/governance/Vesting/VestingRegistryLogic.sol Outdated Show resolved Hide resolved
scripts/contractInteraction/staking_vesting.py Outdated Show resolved Hide resolved
Copy link
Contributor

@tjcloa tjcloa left a comment

Choose a reason for hiding this comment

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

pls complete all conversations

@tjcloa tjcloa changed the title [Consolidation] Staking Rewards PR#448 & Fix Governance Withdraw Vesting PR #448 [Consolidating] PR#451: Incorporate PR#447 Governance vesting cancelling & PR#448 Staking refactoring Oct 26, 2022
Copy link
Contributor

@ororopickpocket ororopickpocket left a comment

Choose a reason for hiding this comment

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

I'm approving this already, but please change the type from uint120 to a power of 2.

@cwsnt cwsnt requested a review from tjcloa November 9, 2022 03:03
vesting = Contract.from_abi("VestingLogic", address=vestingAddress, abi=VestingRegistryLogic.abi, owner=conf.acct)
Copy link
Contributor

Choose a reason for hiding this comment

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

while it will work, the ABI used in confusing
pls change for VestingLogic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

Copy link
Contributor

@tjcloa tjcloa left a comment

Choose a reason for hiding this comment

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

approved
but pls address the last comment on a confusing ABI in a script reading the tokenOwner from vestings

@cwsnt cwsnt merged commit af24e7e into refactoring/staking-contract-eip170 Nov 10, 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.

3 participants