Proposal: General approval pattern (for modular systems and session wallets)

[alvrs]

Note: I’ll use keccak(a,b,c) in this proposal to refer to keccak256(abi.encode(a,b,c))

Problem description

This proposal is a solution for multiple problems:

1. Session wallets:
   Currently, we mainly use burner wallets to reduce the friction of manually approving each transaction. However, this approach is not suitable for storing valuable or permanent assets because the private key of the burner wallet is stored in the browser's local storage and is therefore vulnerable to phishing or loss. The current alternative is to use regular wallets such as MetaMask, which drastically increases the friction of using tx-heavy applications because each tx must be approved. We need a way to store valuable and permanent assets in a main wallet, but interact with the application via a temporary session wallet.
2. Modular systems:
   Described in more detail in Modular systems / system to system calls #319. Short summary: Currently, systems can only serve as an entry point into the application, but it is inconvenient to use systems as stateful libraries or modules to build other higher level systems.

Proposal description

On a high level, the idea is to create a mechanism to allow any address to approve any other address to call certain systems on its behalf.

It is inspired by ERC20’s approve / transferFrom pattern, @dk1a’s Subsystem proposal (#319 (comment) / #268) and @dk1a's comment about approvals here: #319 (comment)

Changes to World contract

We add an ApprovalComponent and ApprovalSystem to the MUD World contract.

• ApprovalComponent

  • EntityID: keccak(grantor, grantee) (for generic approvals) or keccak(grantor, grantee, systemID) (for approvals restricted to certain systems)

  • Value: Approval

    struct Approval {
       uint128 expiryTimestamp;
       uint128 numCalls;
       bytes args;
    }

    • (Side note: To save gas when verifying approvals, expiryTimestamp and numCalls could be bitpacked into a single storage slot. To save even more gas, one bit in this bitpacked slot could be reserved as a flag to represent whether the args value is empty or non-empty.)

• ApprovalSystem

  • Has write access to ApprovalComponent
  • Functions:

    • setApproval(address grantee, uint256 systemID, Approval memory approval)

      • Creates a new approval from msg.sender as grantor to grantee. To create a generic approval, systemID can be set to 0.

    • reduceApproval(address grantor, address grantee, bytes args)

      • Throws if no valid approval is found, reduces numCalls if applicable (see below pseudo code for details)

        function reduceApproval(address grantor, address grantee, bytes memory args) public {
        	// Check for a generic approval
        	(Approval memory approval, bool found) = getApproval(grantor, grantee);
        	
        	// Return successfully if a valid approval is found
        	if(found && approval.expiryTimestamp > block.timestamp) return;
        
        	// Check for a specific approval
        	uint256 systemID = getIdByAddress(msg.sender); // Only the approved system can reduce the approval's numCalls value
        	(approval, found) = getApproval(grantor, grantee, systemID);
        
        	// Trow if no approval is found
        	require(found, "no approval");
        
        	// Throw if expiry timestamp is in the past
        	require(approval.expiryTimestamp > block.timestamp, "approval expired");
        
        	// Throw if numCalls is 0
        	require(approval.numCalls > 0, "no approved calls left");
        
        	// Throw if args exists and doesn't match approved args
        	require(approval.args.length == 0 || approval.args == args, "args not approved");
        
        	// Reduce numCalls, unless it's MAX_UINT128 to support permanent approvals
        	if(approval.numCalls != MAX_UINT128) {
        	     approval.numCalls--;
        	     setApproval(grantor, grantee, approval);
        	}
        }

    • revokeApproval(address grantee) / revokeApproval(address grantee, uint256 systemID)

      • Removes the approval for the given grantee / systemID

    • More functions like checkApproval can be added, but are less relevant for this proposal

Changes to Systems

Every system implements its logic in an internal _execute(address from, bytes memory args) function.

The base System contract implements the following functions:

• execute(bytes memory args): executes the system as msg.sender

  function execute(bytes memory args) public {
  	return _execute(msg.sender, args);
  }

• executeFrom(address from, bytes memory args): checks if msg.sender has approval from from to call the system, then executes the system as from

  function executeFrom(address from, bytes memory args) public {
  	ApprovalSystem.reduceApproval(from, msg.sender, args);
  	return _execute(from, args);
  }

• executeInternal(address from, bytes memory args): checks if msg.sender has approval from address(this) to call this system. If so, the call is trusted and the system is executed as from

  function executeInternal(address from, bytes memory args) public {
  	ApprovalSystem.reduceApproval(address(this), msg.sender, args);
  	return _execute(from, args);
  }

Applications

Session wallets

• At the beginning of each session, the user gives a generic (or restricted) approval with expiryTimestamp to a burner wallet.
• The client calls all systems via their executeFrom functions as the burner wallet, with from set to the main wallet. All assets are owned by the main wallet.
• After the session, the main wallet can revoke the approval, or the approval runs out after the expiryTimestamp

Contract interaction

• Similar to ERC20’s approve / transferFrom pattern, approvals can be used for atomic interactions with contracts, like
  1. User approves a contract to call a TransferSystem once with a certain argument to transfer ownership of a certain item to the contract.
  2. User calls the contract’s swap function, which calls TransferSystem on the user’s behalf to transfer the item to the contract, and then calls TransferSystem on its own behalf to transfer another item from the contract to the user. (Note how this is not possible if TransferSystem only uses msg.sender for access control.)

First party modular systems / using systems as stateful libraries

• Developer wants to modularize movement logic into a MoveSystem, and use it as building block in a PathSystem and CombatSystem (since it’s the same developer, there is full trust in PathSystem and CombatSystem)
• MoveSystem can create a permanent generic approval for PathSystem and CombatSystem, to allow both to call MoveSystem's executeInternal function. (Just like a library, except component access control happens on the level of MoveSystem instead of PathSystem/CombatSystem)
• This use-case and usage is very similar to the approach explored in @dk1a’s subsystems Add subsystem #268
• Creating these permanent generic approvals between first party systems can be automated via the MUD CLI / deploy process.

Third party modular systems

• Assume a third party developer wants to create a new higher level system by combining multiple existing lower level systems.
• They can use the lower level system’s executeFrom functions to do so.
  • This requires users to explicitly approve the higher level system to call the lower level systems on the user’s behalf (with optional restrictions), which is intended to prevent systems from being able to call any system on behalf of users.
  • The approval process can be a default client pop-up integrated into MUD.
• Example:
  • There is a MoveSystem deployed by an unknown developer.
  • A third party developer wants to add a new PathSystem to automate pathfinding and travelling larger distances.
  • PathSystem declares MoveSystem as dependency, client automatically shows a pop up for user to give necessary permissions the first time user attempts to call PathSystem.
    • User calls RegisterSystem.setApproval(PathSystem, MoveSystemID) to allow the PathSystem to permanently (or with restrictions) call MoveSystem on user’s behalf.
  • From now on, PathSystem can call MoveSystem.executeFrom on behalf of the user to automate movement.

[dk1a]

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

[dk1a]

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)

[dk1a]

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)

[agostbiro]

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

[dk1a]

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

[alvrs]

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)).

[alvrs]

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.

[dk1a]

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).

[Kooshaba]

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?

[dk1a]

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 feat(std-contracts): add SystemCallbackComponent #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)

[Kooshaba]

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.