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

Harmonize with roles #22

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Harmonize with roles #22

wants to merge 10 commits into from

Conversation

cristovaoth
Copy link

@cristovaoth cristovaoth commented Jan 21, 2022

This is WIP merge request. Some remarks:

  • Bring in Structures Clearance and ExecutionOptions
    ** This triggered a rewrite of the main check function. The style used by this function is very similar to style used by the checkTransaction function in roles
  • Adopt similar names for main functions:
    ** allowTarget
    ** revokeTarget
    ** scopeTarget
  • Contrarily to roles, here ExecutionOptions are shared between Clearance.Target and Clearance.Function
    • For this reason, and even tho we could piggyback on allowTarget and scopeTarget to set options for free, I left the setExecutionOptions function as standalone. I think it could become ambiguous otherwise

Open question:

  • There was a dedicated error string for when the fallback was targeted, and not allowed. The custom error was: "Fallback not allowed for this address"
  • Under the current changes, such an error will be treat as any other not allowed function: "error FunctionNotAllowed()"
  • Are we to keep the custom error?

@cristovaoth cristovaoth changed the title First rough sketch of possible changes in order to harmonize with zod… Harmonize with roles Jan 21, 2022
@cristovaoth cristovaoth marked this pull request as draft January 21, 2022 15:55
/// Role not allowed to call this function on target address
error FunctionNotAllowed();

bytes4 internal constant FALLBACK_FUNCTION_SIG = 0x00000000;
Copy link
Member

Choose a reason for hiding this comment

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

How to disambiguate between the fallback function and a function with the signature 0x00000000?

Copy link
Author

Choose a reason for hiding this comment

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

I thought they were the same? I never thought about this, but this is true, technically there could be some signature out there that yields 0x00000000

I chatted about this Adam, and he agreed that in roles we would handle the fallback function case via this function signature, the bytes4(0)

contracts/ScopeGuard.sol Outdated Show resolved Hide resolved
@cristovaoth cristovaoth marked this pull request as ready for review January 21, 2022 22:11
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