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

add Execution #67

Merged
merged 46 commits into from
Apr 15, 2022
Merged

add Execution #67

merged 46 commits into from
Apr 15, 2022

Conversation

pscott
Copy link
Contributor

@pscott pscott commented Mar 28, 2022

This PR adds the execution feature.
Basically, this PR adds a finalize_proposal method to the space contract, and adds a zodiac_relayer.cairo contract that will relay the execution to L1. L1 smart-contract is also modified to have a mapping of whitelisted Spaces that can trigger the execution.

This finalize_proposal can be called by anyone, but only after the proposal_period has ended.
It sends a message to the execution contract stored in executor, and notes the proposal as executed. It is up to the execution contract to decide what to do with the information provided.

@pscott
Copy link
Contributor Author

pscott commented Apr 1, 2022

This PR is finally ready!
The tests are failing, but they are failing on another PR too... (for some reason it looks like starknet-devnet can't connect to port 8000? will have to check)

@pscott pscott marked this pull request as ready for review April 1, 2022 15:24
Comment on lines 109 to 110
# TODO: L1 needs to know about L2 address, but L2 needs to know about the L2 address... need to fix that.
# TODO: this should either be on the l1 contract or the l2 contract. Since l1 contract has an `owner` I think it should be on l1.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This needs to be addressed (we can merge like this, but will definitely need to be addressed soon)

Comment on lines 55 to 56
// This should be declared along with the other const but doing so will make the compiler unhappy as `SplitUin256`
// will be undefined for some reason?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need some TS gods to help me here... initially had it along with the other consts but it simply didn't work ^^

@pscott
Copy link
Contributor Author

pscott commented Apr 4, 2022

Currently working on adding:

  • An interface for the execution
  • storing the execution parameters (as a Hash) and recovering it
  • A zodiac_l1 execution strategy
  • Adding a check on the l1 zodiac module to add a whitelist of accepted callers

@Orland0x
Copy link
Contributor

Im wondering whether the entire proposal should be cancelled if overflow on the voting power count happens rather than just making that vote fail. The proposal is broken if it happens so seems like it should be cancelled

@Orland0x Orland0x merged commit d4b2652 into develop Apr 15, 2022
@Orland0x Orland0x deleted the add_execution branch May 4, 2022 13:33
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