Proposal: General approval pattern (for modular systems and session wallets) about mud HOT 22 CLOSED

Hi,

First time posting. I love this project and I've been following the issues. I'm very impressed by the quality of discussions here and this proposal in particular.

I wanted to share that I'm working on a better MetaMask that can do automated transaction approval in an EIP-1993 compatible way by creating a new key for each dapp that the user connects. It works the same way as burner wallets from the dapp perspective, but the keys are created and stored in the wallet app (instead of the page's JS context) and synced across the user's devices. You can find out more about how this works in the user docs and in the technical docs.

Obviously, it's not a solution for you problem today, just wanted to give you context that changes are to be expected in how wallets work in the future.

Best,
Agost

from mud.

sealvault looks really useful.
It protects against malicious dapps in general,
whereas approval system protects against malicious third-party systems within a mud-based dapp.
To me the 2 actually seem complementary, as they solve different (though related) problems

from mud.

I wanted to share that I'm working on a better MetaMask that can do automated transaction approval in an EIP-1993 compatible way
- #327 (comment)

Thanks for sharing this context! I agree with @dk1a that sealvault looks useful and is complementary to this issue.

from mud.

The relationship between ItemExtractorSystem with MatchItemExtractionSystem would be along the same lines, with MatchItemExtractionSystem creating an approval for ItemExtractorSystem to allow it to be called like a subsystem.
#327 (comment)

I had thought it was intentional, allowing not-fully-trusted third-party sky strife clients for amalgema or something like that. But otherwise executeInternal would fit well indeed.

from mud.

Started an implementation in #345, it should help discussing the finer details.
I outlined some points that came to mind there.
Check out its general architecture first though, if you want drastic changes, that may make all my smaller questions invalid

from mud.

A really good proposal! Much better thought out than my vague approval concept, and very generalizable unlike subsystems.

For posterity: my comment about approvals was itself inspired by my OperatorApprovalComponent for ERC721/1155 Systems, so it all comes from ERC20 after all

Minor concern: permanent approvals can be impossible to revert if you forget who you approved it for (since you can neither reverse the hash nor loop through all addresses), but this can be solved via an extra component, or discouraging such approvals. (ERC20 etc solve this via events having the data - the extra component could hold such data instead)

executeInternal is special. Fully internal systems (subsystems) would want to disable execute and executeFrom, because address doesn't matter - (stateful) libs often operate on "ownerless" entities.
E.g. CombatSubsystem receives 2 entities:
1 of which can be an NPC (which has no owner at all),
and another kind of has an owner, but then that owner should never be able to call CombatSubsystem directly.

To that end I can still see a use case for a Subsystem, in the sense that allowing overrides of execute and executeFrom to disable them seems bad. I'm also not sure executeInternal is even needed in normal systems.
(and the Subsystem name still provides the benefit of working with it in cli easier, and readability of what's internal)
And if the Subsystem ends up existing separately anyways, it might easier to do it via OwnableWritable after all. But that's just an implementation detail at that point, I don't yet have an opinion either way

from mud.

Just thought of another use-case for AbstractComponent from #267 while sketching some approval stuff.
You could use it to store structs in an optimal way, e.g.

contract ApprovalComponent is AbstractComponent {
  /** Mapping from entity id to value in this component */
  mapping(uint256 => Approval) internal entityToValue;

  constructor(address world, uint256 id) AbstractComponent(world, id) {}

  function getSchema() public pure override returns (string[] memory keys, LibTypes.SchemaValue[] memory values) {
    ...
  }

  function set(uint256 entity, Approval memory value) public virtual onlyOwner {
    _set(entity, value);
  }

  function _set(uint256 entity, bytes memory value) internal override {
    _set(entity, abi.decode(value, (Approval)));
  }

  function _set(uint256 entity, Approval memory value) internal virtual {
    // Store the entity's value;
    entityToValue[entity] = value;

    // Emit global event
    IWorld(world).registerComponentValueSet(entity, abi.encode(value));
  }

  function _remove(uint256 entity) internal virtual override {
    // Remove the entity from the mapping
    delete entityToValue[entity];

    // Emit global event
    IWorld(world).registerComponentValueRemoved(entity);
  }

  function has(uint256 entity) public view virtual override returns (bool) {
    return entityToValue[entity].numCalls != 0;
  }

  function getValue(uint256 entity) public view virtual returns (Approval memory) {
    return entityToValue[entity];
  }

  function getRawValue(uint256 entity) public view virtual override returns (bytes memory) {
    return abi.encode(entityToValue[entity]);
  }
}

This is sort of a bespoke alternative to bitpacking, and even more efficient in many cases (doesn't store length, which is 1 less slot; not sure how decode vs encode compares gas-wise).
(mud doesn't care much about gas atm, which is fine cause premature optimization is bad, but it becomes more meaningful down the line, especially for something pervasive like approval)

from mud.

Also a note on overriding _execute:

Can't speak on general ApprovalSystem, which doesn't exist yet. In theory seems fine.

But for subsystems by now I actually dislike that part, in practice having no default execute seems more useful.
This is because my library-like systems tend to have several sub-executes that use similar dependencies, but are themselves unrelated.
E.g. CombatSubsystem has executeCombatRound, executeActivateCombat, executeDeactivateCombat. Neither of those 3 call each other, and the main execute just routes to the selected sub-execute. Having sub-execute -> execute -> internal sub-execute would just make it all way harder to read. And having 3 systems would be very inconvenient as well (it's not just CombatSubsystem, I'd get like 10 more subsystems)

Maybe a fully automated deployment would make having a ton of internal systems non-painful (#295 has some notes on why full automation is hard and unsolved)

from mud.

permanent approvals can be impossible to revert if you forget who you approved it for
- #327 (comment)

Very good point! We could also account for this by storing this information in the Approval component itself:

struct Approval {
   uint128 expiryTimestamp;
   uint128 numCalls;
   bytes args;
   address grantor;
   address grantee;
   uint256 systemID;
}

The Approval component would probably be a BareComponent because the reverse mapping is not really useful in this case, but adding information about grantor, grantee and optional systemID to the struct would cause it to be emitted via the ComponentValueSet event, so it would be reconstructable.

executeInternal is special. Fully internal systems (subsystems) would want to disable execute and executeFrom, because address doesn't matter - (stateful) libs often operate on "ownerless" entities.
[...]
I'm also not sure executeInternal is even needed in normal systems.
- #327 (comment)

Also a note on overriding _execute:
Can't speak on general ApprovalSystem, which doesn't exist yet. In theory seems fine.
But for subsystems by now I actually dislike that part, in practice having no default execute seems more useful.
- #327 (comment)

Yeah, this is something I also thought about when drafting this proposal. The original idea behind requiring a default execute function in systems was to create dynamic UI on the client without requiring the system's ABI - but this was 1. never implemented and 2. by now I'm not convinced anymore that having a default execute method would be a good approach for that. The other second order advantage of having a default execute method is to encourage devs to split up logic into modular systems, but maybe that should be more or a convention than a rule. (In practice nothing prevents devs from registering any contract with any interface as "System" in the World contract anyway.) So in short, I'm all for removing execute as required from the System interface.

Instead, we could implement the access control checks described above in executeFrom and executeInternal as modifiers (eg onlyApproved, onlyApprovedByThis), and let devs add any number of xxxFrom / xxxInternal functions as needed. (And and make the functionFrom / functionInternal naming a convention rather than a requirement we can't enforce).

modifier onlyApproved(address grantor, bytes memory args) {
   ApprovalSystem.reduceApproval(grantor, msg.sender, args);
   _;
}

modifier onlyApprovedByThis(bytes memory args) {
   ApprovalSystem.reduceApproval(address(this), msg.sender, args);
   _;
}

To improve the experience for devs who want to use the execute pattern, we can add a ExecuteSystem that implements the _execute, execute, executeFrom, executeInternal functions using above modifiers with the spec described in the initial message (#327 (comment)).

from mud.

We could also account for this by storing this information in the Approval component itself
#327 (comment)

The reason I leaned into a 2nd component initially was because that info is stored once, and then rarely used once to cancel. Whereas numCalls is used all the time. So it's just a gas tradeoff, doesn't really matter.

I'm all for removing execute as required from the System interface.
#327 (comment)

Haha you went farther than I meant, but I agree of course (I just meant the modifiers part of it, not removing execute from ISystem).
I can't think of any reason why many methods would be bad. I already actively do that in my systems. (#303 depends on execute, but it isn't even merged, and can be easily changed to support arbitrary methods)

This maybe also helps with something like #325, where execute gets in the way (are read-only systems better than FunctionComponent sometimes?)

To improve the experience for devs who want to use the execute pattern, we can add a ExecuteSystem that implements the _execute, execute, executeFrom, executeInternal functions

While I'm still skeptical about executeInternal there (it doesn't really hurt tho), it's totally worth it for the execute+executeFrom, those seem like a useful default to have in most places; the execute pattern just saves some boilerplate for 1-function systems.

Those comments are based on my project's current design:
All external systems are 1-function (would be execute+executeFrom in the new paradigm).
All internal systems are multi-function (would be multiple different executeInternals).
No systems are hybrid (i.e. none would need execute+executeFrom+executeInternal).

from mud.

I have a very specific use-case that I am working through right now that requires this feature, so I'd like to explain the player journey to see if I understand everything correctly.

The simple explanation is that I need two systems that exist on separate MUD Worlds to be able to interact with each other and transfer information between them in a trusted way.

Here is the specific example of how Sky Strife would use this:

Player Journey:

• player registers for a Sky Strife match
• during registration:
  • Approval is created for the Amalgema JoinMatchSystem to impersonate them (1 call)
• player calls JoinMatchSystem, which calls MacroJoinMatchSystem and joins match
• during join:
  • Approval is created for the Sky Strife ItemExtractionSystem to impersonate them (1 call)
• player plays match and accumulates items
• on match end, player calls ItemExtractionSystem, which calls MatchItemExtractionSystem, and transfers items to Amalgema

If I am understanding everything correctly, this proposal fits my use-case and I see no issues 👍🏼

I do have a question about internal systems though. I've been using public libraries in the same way that @dk1a described, so I don't see a need for the internal systems. Is there something that public libraries cannot do that the internal systems can?

from mud.

If I am understanding everything correctly, this proposal fits my use-case and I see no issues 👍🏼
#327 comment

Looks good to me too

Is there something that public libraries cannot do that the internal systems can?

I have considered public libs as a workable alternative, mostly they can do what internal systems can.
Exceptions:

• Callbacks, but I use those only for 1 internal system.
• ERC721/1155 Subsystems can't be libs, but they're special and I wouldn't need a concept of "internal system" just for them.

I have a bias against public libs because I don't like how they fit into the deployment pipeline.
I am curious to see how you do it in skystrife, maybe it's not all as bad as I imagine.
Also these points from my writeup on benefits of subsystems over internal libs apply to public libs as well:

3. Callbacks (see #303) are impossible with libs.
4. I don't even know how many components CycleCombatSystem uses if you unroll the subsystems. Manually managing huge writeAccesses is very inconvenient.
5. Libraries don't have writeAccess. Subsystems do. You can easily see which components/subsystems a subsystem writes to. You have to read the whole library to know what it writes to.
6. Statefulness can be convenient. I don't need to pass world or components to a subsystem.

#319 comment

(the last 2 points are very minor and wouldn't have made a difference on their own)

from mud.

As far as deployment I've been using the default MUD deploy using forge and everything just works out of the box. I believe the only small issue is that it deploys a new library every time it is referenced in a system.

I agree that managing write access is pretty nice with subsystems. I currently avoid the entire problem and just set writeAcess: * in all my systems 😅 Not ideal.

from mud.

Player Journey:

Player Journey:

• player registers for a Sky Strife match
• during registration:
  • Approval is created for the Amalgema JoinMatchSystem to impersonate them (1 call)
  • player calls JoinMatchSystem, which calls MacroJoinMatchSystem and joins match
• during join:
  • Approval is created for the Sky Strife ItemExtractionSystem to impersonate them (1 call)
  • player plays match and accumulates items
  • on match end, player calls ItemExtractionSystem, which calls MatchItemExtractionSystem, and transfers items to Amalgema

- #327 (comment)

If I understand the flow correctly, I think MacroJoinMatchSystem (grantor) could just create an approval for JoinMatchSystem, essentially turning MacroJoinMatchSystem into a "Library" for JoinMatchSystem. With the spec from the initial issue, this would mean JoinMatchSystem calls MacroJoinMatchSystem via executeInternal, so it can pass any from address and MacroJoinMatchSystem trusts it (which is fine because they're both coming from the same developer and JoinMatchSystem's code can be verified to just forward its msg.sender (or it's from to be preceise)).

The relationship between ItemExtractorSystem with MatchItemExtractionSystem would be along the same lines, with MatchItemExtractionSystem creating an approval for ItemExtractorSystem to allow it to be called like a subsystem.

In other words, the flow you described sounds like a use case for executeInternal / "Subsystem" more than for impersonating players.

Note: the flow you described would also work, but would require each player to approve the JoinMatchSystem to call the MacroJoinMatchSystem on their behalf, and then would require JoinMatchSystem to call MacroJoinMatchSystem via its executeFrom method.

However, there would also be a use case for the executeFrom / "impersonation" / "session wallets" in Sky Strife:

• Amalgema is aware of user's "main wallet", items are associated with the main wallet, crafting etc. in the Amalgema client requires players to approve a transaction
• When a Sky Strife match is joined:
  • The flow described above happens
  • The Sky Strife client creates a burner wallet for the match
  • The user creates an Approval for this burner wallet in the Sky Strife world once.
• The Sky Strife client calls all systems via their executeFrom method via the burner wallet, passing the user's main wallet as from parameter. ("The burner wallet impersonates the main wallet"). Sky Strife's internal logic (ie the systems' _execute methods) doesn't have to be aware of this, they just trust their from parameter, and the default executeFrom method (or modifier) handles the access validation.

from mud.

How about MPC wallet ？

from mud.

How about MPC wallet ？
- #327 (comment)

@Osub can you elaborate?

from mud.

MPC wallets are multi-party computing shared key fragments that can interact with contracts using 2 of 3 signatures. part1 exists client, part2 game operation, part3 user backup. Interact with the contract using part1 and part2 co-signatures. The user registers the game operation server with a social account or mobile phone number, refer to https://docs.particle.network/

from mud.

For account delegation/session wallets/burner wallets, I wonder if we should use something more general purpose/widely supported, that way more things outside of MUD can plug into it directly, e.g. https://delegate.cash/

from mud.

For account delegation/session wallets/burner wallets, I wonder if we should use something more general purpose/widely supported, that way more things outside of MUD can plug into it directly, e.g. https://delegate.cash/
- #327 (comment) by @holic

Valid point! If we go with a central entry point (as discussed in #347) we could also easily provide multiple entry points for different account abstraction methods, without requiring system developers to think about it / to upgrade (they can just trust that the _from / _msgSender param passed by the World is authenticated with some valid method)

from mud.

Started an implementation in #345, it should help discussing the finer details.
I outlined some points that came to mind there.
Check out its general architecture first though, if you want drastic changes, that may make all my smaller questions invalid
- #327 (comment) by @dk1a

Wow very cool! Lots of good points in the PR description.

However I wonder if we should delay this issue until we settled on sth in #347 (since it impacts a lot of code that would be written for this one) and adjust this proposal for #347 when the time has come? #347 will obviously be a breaking v2 change, but I wonder if it's worth putting effort in a v1 approval system (and requiring developers to upgrade all their systems if they want to use it) instead of waiting for v2 (which still requires devs to upgrade, but at least only once, and then things like the approval pattern can be implemented in a non-breaking way in the central World entry point).

from mud.

However I wonder if we should delay this issue until we settled on sth in #347
#327 (comment)

I had the same thoughts after reading the proposal.
Personally I much prefer the more thorough changes and simply didn't expect the v2 data model to be coming soon. I'll close #345, as I'd rather contribute to v2. If someone wants to continue #345 feel free to copy my changes

from mud.

Closing this in favor of #1124

from mud.