Comments (22)
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:
- Callbacks (see #303) are impossible with libs.
- I don't even know how many components CycleCombatSystem uses if you unroll the subsystems. Manually managing huge writeAccesses is very inconvenient.
- 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.
- Statefulness can be convenient. I don't need to pass world or components to a subsystem.
(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
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 asfrom
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 theirfrom
parameter, and the defaultexecuteFrom
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.
Related Issues (20)
- error msg couldn't be seen in client dev
- remove `typesVersions` from package.json
- add tests that checks if a table is registered when used
- add base `MudScript` to inherit from HOT 1
- World Explorer ultimate task
- revisit config key errors with arktype's `stringifyUnion`
- Dev tools tx simulation reverts if value is required
- Enable JS tracing on RPC HOT 4
- throw instead of truncating namespaces
- Can we have a `forge options` command line option on `mud deploy`? HOT 1
- Out of gas during deploy to conduit chains HOT 2
- loosen solidity version requirements
- improve error when system name != filename
- throw namespace length error during config validation
- explore writing config output to strongly typed json file
- upgrade rimraf everywhere
- enable type checking in template tests HOT 1
- Add type tests for inferring `recs` components from the config
- remove `getSrcDirectory`
- revisit module args in config
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from mud.