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

Implement Access Control #249

Closed
martriay opened this issue Mar 30, 2022 · 11 comments · Fixed by #373
Closed

Implement Access Control #249

martriay opened this issue Mar 30, 2022 · 11 comments · Fixed by #373

Comments

@martriay
Copy link
Contributor

No description provided.

@andrew-fleming
Copy link
Collaborator

The Solidity implementation for AccessControl stores the roles as type bytes32. Cairo field elements, however, can only store 252 bits (31.5 bytes). For interoperability, it seems we cannot use a regular felt to store the role on StarkNet. One potential solution is to store the role as a mapping of two felts. @frangio @Amxx opinions on best way to proceed?

@milancermak
Copy link
Contributor

@andrew-fleming What interoperability do you have in mind in this case?

@Amxx
Copy link
Contributor

Amxx commented Jun 13, 2022

At first, the PR proposed (#239) used a uint256. If was felts that felt would be much easier to manipulate with than uint256.

IMO, using 252 bits for roles is way sufficient to avoid collisions. It has the drawback that you can't simply reuse the bytes32 roles transparently, but I don't think it's an issue. We can "just" take 252 bits of keccak256("PROPOSER_ROLE") and be good with it

@andrew-fleming
Copy link
Collaborator

@milancermak one specific example (that's fresh because I'm working on it) is with standard Timelock roles a la keccak256("TIMELOCK_ADMIN_ROLE"), keccak256("PROPOSER_ROLE"), etc.

@milancermak
Copy link
Contributor

Right, but what's the benefit of having the role const the same in a Solidity contract and a Cairo contract?

@andrew-fleming
Copy link
Collaborator

We can "just" take 252 bits of keccak256("PROPOSER_ROLE") and be good with it

😆

That seems reasonable to me^

Right, but what's the benefit of having the role const the same in a Solidity contract and a Cairo contract?

@milancermak ah sorry, I didn't answer your question directly. I think the benefit is in easing the transition for existing applications following the standard. It'd be much simpler, IMO, for cross-chain applications that implement AccessControl to be able to use the same value (thus the same or similar mechanism to calculate said value). Though, @Amxx's solution might resolve this discrepancy with the best of both worlds approach

@Amxx
Copy link
Contributor

Amxx commented Jun 13, 2022

Note, if you have an app that lives both in the EVM and Starknet spaces, you can stick to 252 bits roles and use them in both places.

@frangio
Copy link
Contributor

frangio commented Jun 13, 2022

I would definitely prioritize usability over strict compatibility with Solidity. What is the size of hashes in Cairo natively? If felt, I'd say let's use felt.

@martriay
Copy link
Contributor Author

martriay commented Jun 14, 2022

We can use felt, but what should we represent in them? Should it be the first or last 252bits of keccak256("ROLE") or should we use cairo's own hashing on a short string?

@frangio
Copy link
Contributor

frangio commented Jun 14, 2022

No strong opinion on this. Note that the format of roles is not mandated by AccessControl, it's up to the user and recommendations can change in the future. We've seen users of Solidity's AccessControl use various formats for their role ids including different hash functions or the direct string bytes.

@martriay
Copy link
Contributor Author

We can maintain certain agnosticism then. Let's do felts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants