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

Update Adapter Access Control Logic #458

Merged

Conversation

Kayanski
Copy link
Contributor

@Kayanski Kayanski commented Sep 16, 2024

This PR aims at updating the Access Control for actions via the account.

Proposed changes:

  • Merge All actions on the account (ExecOnModule, ModuleActions) to a single LocalAction variant
    • ExecOnModule simply fetches the module address and dispatches the message
  • Add a ConfigureModule message on the account for sending admin calls to modules
  • Enforce this access control inside Adapter Base Variant
  • Modify the NestedAmin structure to be able to account for this admin change

This is Options 2 here, there is a mermaid diagram at the end to understand the situation a little better https://www.notion.so/abstract-money/Merging-Abstract-Account-Implementation-6d366bb35b3242adb6ae5787fb759cc8?pvs=4

Checklist

  • CI is green.
  • Changelog updated.

@Kayanski Kayanski changed the base branch from main to develop/v2 September 16, 2024 10:47
Copy link

cloudflare-workers-and-pages bot commented Sep 16, 2024

Deploying abstract-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: 74064d3
Status: ✅  Deploy successful!
Preview URL: https://8665d12c.abstract-docs.pages.dev
Branch Preview URL: https://nicolas-abs-514-update-adapt.abstract-docs.pages.dev

View logs

@Kayanski Kayanski marked this pull request as ready for review September 16, 2024 13:58
@Kayanski Kayanski marked this pull request as draft September 16, 2024 14:18
Copy link
Collaborator

@Buckram123 Buckram123 left a comment

Choose a reason for hiding this comment

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

Looks good! Noticed one edge case, which I hope is unlikely to happen

framework/contracts/account/src/actions.rs Outdated Show resolved Hide resolved
framework/contracts/account/src/contract.rs Outdated Show resolved Hide resolved
@CyberHoward
Copy link
Contributor

Namely, today only the admin (might be the account) OR the top level owner can execute on the admin variants. We need to modify the NestedAdmin structure to be able to verify that IF the admin is an account, it should be in an admin state. Is that an acceptable behavior

Yes, I think this is the behavior we want. A nucance here is that we want to do the admin-flag assertion on the direct admin of the app (i.e. the account that the app is installed on) vs the top-level account. Because an admin action on a sub-account should not require an admin flag be set on the owner account.

framework/packages/abstract-std/src/account.rs Outdated Show resolved Hide resolved
framework/packages/abstract-std/src/account.rs Outdated Show resolved Hide resolved
framework/packages/abstract-std/src/account.rs Outdated Show resolved Hide resolved
framework/packages/abstract-std/src/account.rs Outdated Show resolved Hide resolved
framework/packages/abstract-std/src/account.rs Outdated Show resolved Hide resolved
@Kayanski Kayanski marked this pull request as ready for review September 23, 2024 10:17
Copy link
Contributor

@CyberHoward CyberHoward left a comment

Choose a reason for hiding this comment

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

Could you update packages/abstract-std/src/objects/ownership/README.md with an explanation of how the CALLING_TO_AS_ADMIN value is used?

@CyberHoward CyberHoward merged commit f3cbba9 into develop/v2 Oct 2, 2024
21 of 23 checks passed
@CyberHoward CyberHoward deleted the nicolas/abs-514-update-adapter-base-config-logic branch October 2, 2024 11:56
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