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

Whitelist executors #103

Merged
merged 16 commits into from
May 8, 2022
Merged

Whitelist executors #103

merged 16 commits into from
May 8, 2022

Conversation

pscott
Copy link
Contributor

@pscott pscott commented May 3, 2022

This PR:

  • Moves the executor from a single address to a whitelist of executors (provided via constructor, can be modified by the controller using the add_executors and remove_executors)
  • Proposals now are required to have an executor specified (when creating a proposal via propose). This executor needs to be part of the whitelist
  • When calling finalize_proposal, the executor is checked to see whether it's still in the whitelist. If it is not, then the proposal_outcome is set to CANCELLED

Closes #94

@pscott pscott requested a review from Orland0x May 3, 2022 12:03
@pscott
Copy link
Contributor Author

pscott commented May 3, 2022

Side note:
I've created 3 issues that I thought about when writing this code:

@pscott pscott mentioned this pull request May 6, 2022
contracts/starknet/space.cairo Outdated Show resolved Hide resolved
@@ -113,6 +113,38 @@ func update_controller{syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_c
return ()
end

@external
func add_executors{syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr : felt}(
Copy link
Contributor

Choose a reason for hiding this comment

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

can we get away with just add_executors or just register_executors ? seems kind of redundant to have both

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lmao I'm stupid! Ofc we can :) 843ad89

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh actually the reason why there was an add_executors and a register_executors was because add_executors is only_controller() whereas register is not. Since _controller is an argument passed in to the constructor (and not get_caller_address), add_executors fails when called in the constructor... I think we should keep register_executors, but we could decide to go the other way and simply set the _controller to get_caller_address in the constructor

Copy link
Contributor

Choose a reason for hiding this comment

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

ok i see yes. annoying but not much we can do then

Copy link
Contributor

Choose a reason for hiding this comment

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

once we create the space factory we can properly figure out the space deployment/controller update flows

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Decided to fix it by having add_executors simply be a wrapper with a call to only_owner() before register_executors. I think naming should be changed though, and maybe use _ as we sometimes do in solidity :)

2344068

contracts/starknet/space.cairo Show resolved Hide resolved
@@ -113,6 +113,38 @@ func update_controller{syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_c
return ()
end

@external
func add_executors{syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr : felt}(
Copy link
Contributor

Choose a reason for hiding this comment

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

ok i see yes. annoying but not much we can do then

@@ -113,6 +113,38 @@ func update_controller{syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_c
return ()
end

@external
func add_executors{syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr : felt}(
Copy link
Contributor

Choose a reason for hiding this comment

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

once we create the space factory we can properly figure out the space deployment/controller update flows

@pscott pscott merged commit 60de3f2 into develop May 8, 2022
@Orland0x Orland0x deleted the whitelist_executors branch May 9, 2022 13:38
@Orland0x Orland0x restored the whitelist_executors branch May 9, 2022 13:38
@Orland0x Orland0x deleted the whitelist_executors branch May 28, 2022 15:03
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.

Move execution from a per space to a per proposal
2 participants