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

few updates #159

Merged
merged 79 commits into from
Jun 14, 2022
Merged

few updates #159

merged 79 commits into from
Jun 14, 2022

Conversation

Orland0x
Copy link
Contributor

@Orland0x Orland0x commented May 30, 2022

Few updates. Got a bit carried away, will do on separate PRs next time

Allow handling of both Ethereum and Starknet addresses in the space contract and voting strategies.

Switched to a felt to uint256 function from cairo common

Fixed vulnerability in authenticators where the library function execute could be called directly.

Added starknet.js deployment script for a space contract with a number of modules activated. Can call deployment script with yarn deploy:goerli.
Deployment info in ./deployments/goerli1.json

@Orland0x Orland0x marked this pull request as draft May 31, 2022 21:21
@Orland0x Orland0x marked this pull request as ready for review June 1, 2022 21:51
@Orland0x Orland0x requested a review from pscott June 1, 2022 21:51
@Orland0x Orland0x changed the title Generalize addresses few updates Jun 1, 2022
Copy link
Contributor

@pscott pscott left a comment

Choose a reason for hiding this comment

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

All good except one test that got modified :)

Comment on lines +1 to +4
# Generalized type used to represent addresses in Snapshot X. Eg Ethereum, Starknet, etc.
struct Address:
member value : felt
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be called lib/address ? I find it odd to have a filename be general_address and the actual object be Address ?

Copy link
Contributor Author

@Orland0x Orland0x Jun 14, 2022

Choose a reason for hiding this comment

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

hmm yeah its not ideal, just thought that people would assume its a Starknet address if its called address.cairo

.commit(
getCommit(BigInt(space.address), BigInt(getSelectorFromName('vote')), proposeCalldata)
); // Wrong selector
.commit(getCommit(BigInt(space.address), PROPOSE_SELECTOR, proposeCalldata)); // Wrong selector
Copy link
Contributor

Choose a reason for hiding this comment

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

This was supposed to be VOTE_SELECTOR, not PROPOSE_SELECTOR I think (since the comment says "Wrong selector") ?

Copy link
Contributor Author

@Orland0x Orland0x Jun 14, 2022

Choose a reason for hiding this comment

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

The test was testing that revert case (ie authentication fails if the wrong hash is committed) - the comment is just referring to what I changed to cause it to fail.

Comment on lines +21 to 41
let spaceAddress: bigint;
let executionHash: string;
let metadataUri: bigint[];
let proposerAccount: Account;
let proposerAddress: string;
let usedVotingStrategies1: bigint[];
let userVotingParamsAll1: bigint[][];
let executionStrategy: bigint;
let executionParams: bigint[];
let ethBlockNumber: bigint;
let proposeCalldata: bigint[];

// Additional parameters for voting
let voterAccount: Account;
let voterAddress: string;
let proposalId: bigint;
let choice: Choice;
let usedVotingStrategies2: bigint[];
let userVotingParamsAll2: bigint[][];
let voteCalldata: bigint[];

Copy link
Contributor

Choose a reason for hiding this comment

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

nice and tidy

@Orland0x Orland0x merged commit 5eccb74 into develop Jun 14, 2022
@Orland0x Orland0x deleted the generalize_addresses branch July 25, 2022 09:27
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