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

feat: add ApprovalSystem #345

Closed
wants to merge 1 commit into from
Closed

Conversation

dk1a
Copy link
Contributor

@dk1a dk1a commented Jan 14, 2023

Draft for #327

Particular things I'd like feedback on:

  1. ExternalSystem is meant to be temporary to avoid breaking changes in this PR. In v2 I want to rename System to BareSystem/BaseSystem, and ExternalSystem to System.
  2. requireApproved and onlyApproved probably shouldn't both exist. I prefer requireApproved, since modifiers are in various ways inferior to funcs (no explicit visibility; can't use computed args; only 1 modifier can have a name regardless of args).
  3. I want world to be more modular. It just links to IApprovalSystem, which then has all the methods. The intent is to have this be the new standard, and later refactor RegisterSystem to follow suite.
  4. ApprovalComponent's getValue never throws. ApprovalSystem has no separate error for absent approval, which reuses ApprovalSystem__TimeExpired. (to avoid calling has, then getValue. I think this is shorter, easier to read, and probably saves gas on a method call).
  5. ApprovalEntityReversal is separate from Approval. You'll see in ApprovalSystem that reversal is used only once, so in most cases it would needlessly complicate approval if combined together.
  6. After making approval-based Subsystem and comparing to Add subsystem #268, I prefer Add subsystem #268, at least in their current forms.
    • Approval has rather complicated logic, designed for mostly non-permanent approval. Whereas using OwnableWritable mirrors components, designed to be permanent, without vestigial data like expiryTimestamp.
    • Approval is client-sourced, whereas writeAccess is cli-sourced (as with components).
  7. Should this PR have clientside stuff too, or would you rather merge solecs changes first?
  8. Should generic approvals have their own component without args and numCalls? (that felt like overkill)
  9. On the other hand, does IApprovalSystem need separate methods for generic approvals, or should ppl just pass in 0?
  10. ApprovalSystem's execute is a deprecated noop. This assumes v2 will remove execute from ISystem.

(no particular order, numbers can make responding easier. Any other feedback is welcome too, this is all subject to change)

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.

1 participant