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

fix a couple nits with the interfaces and code #37

Merged
merged 3 commits into from
Apr 23, 2024
Merged

fix a couple nits with the interfaces and code #37

merged 3 commits into from
Apr 23, 2024

Conversation

moodysalem
Copy link
Member

No description provided.

@moodysalem moodysalem merged commit 6ccf304 into main Apr 23, 2024
1 check passed
@moodysalem moodysalem deleted the nits branch April 23, 2024 18:11
Copy link
Member Author

@moodysalem moodysalem left a comment

Choose a reason for hiding this comment

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

marked up with rationale for changes

Comment on lines +49 to +53
// Cancel the proposal with the given ID. The proposal may be canceled at any time before it is executed.
// There are two ways the proposal cancellation can be authorized:
// - The proposer can cancel the proposal
// - Anyone can cancel if the average voting weight of the proposer was below the proposal_creation_threshold during the voting period (at the given breach_timestamp)
fn cancel_at_timestamp(ref self: TContractState, id: felt252, breach_timestamp: u64);
Copy link
Member Author

Choose a reason for hiding this comment

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

added this method so that a proposal can be canceled by anyone if the proposer's voting weight dips below the threshold at any time t between when the proposal is created and when the proposal ends

@@ -100,6 +105,7 @@ pub mod Governor {
#[derive(starknet::Event, Drop)]
pub struct Canceled {
pub id: felt252,
pub breach_timestamp: u64,
Copy link
Member Author

Choose a reason for hiding this comment

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

previously we knew it was always the block in which the event was emitted

since it's no longer true, we need to emit it from the event to know

Comment on lines +265 to +270
assert(
breach_timestamp < (proposal.execution_state.created
+ config.voting_start_delay
+ config.voting_period),
'VOTING_ENDED'
);
Copy link
Member Author

Choose a reason for hiding this comment

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

note that the proposal can still be canceled after voting ends, but before it is executed

executed: 0,
canceled: timestamp_current
canceled: get_block_timestamp()
Copy link
Member Author

Choose a reason for hiding this comment

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

still, we use the current timestamp for the time at which it was canceled

Comment on lines +16 to +23
// Transfer the specified amount of token from the caller into this contract and delegates the voting weight to the specified delegate
fn stake_amount(ref self: TContractState, delegate: ContractAddress, amount: u128);

// Unstakes and withdraws all of the tokens delegated by the sender to the delegate from the contract to the given recipient address
fn withdraw(ref self: TContractState, delegate: ContractAddress, recipient: ContractAddress);

// Unstakes and withdraws the specified amount of tokens delegated by the sender to the delegate from the contract to the given recipient address
fn withdraw_amount(
Copy link
Member Author

Choose a reason for hiding this comment

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

these improvements to the interface allow you to approve once and then stake for multiple delegates, or move a partial delegation

Comment on lines +226 to +231
self
.token
.read()
.allowance(get_caller_address(), get_contract_address())
.try_into()
.expect('ALLOWANCE_OVERFLOW')
Copy link
Member Author

Choose a reason for hiding this comment

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

if called without an amount argument, stakes the entire approval

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.

1 participant