-
Notifications
You must be signed in to change notification settings - Fork 184
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(world-module-erc20): new ERC20 World Module #3300
Conversation
🦋 Changeset detectedLatest commit: e44b98b The changes in this PR will be included in the next version bump. This PR includes changesets to release 25 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall very nice, very excited about this!
if (!ResourceAccess.get(getNamespaceId(), sender)) { | ||
revert CallerHasNoNamespaceAccess(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could technically reuse the AccessControl
library here:
mud/packages/world/src/AccessControl.sol
Lines 51 to 56 in 3a80bed
function requireAccess(ResourceId resourceId, address caller) internal view { | |
// Check if the given caller has access to the given namespace or name | |
if (!hasAccess(resourceId, caller)) { | |
revert IWorldErrors.World_AccessDenied(resourceId.toString(), caller); | |
} | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but this would make it harder to see where the error comes from 🤔 I'm fine with either, which ever is more standard across the codebase
function namespaceId() internal pure returns (ResourceId) { | ||
return WorldResourceIdLib.encodeNamespace(NAMESPACE); | ||
} | ||
|
||
function registryTableId() internal pure returns (ResourceId) { | ||
return WorldResourceIdLib.encode(RESOURCE_TABLE, NAMESPACE, REGISTRY_TABLE_NAME); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If these are only used during the installation leaving and not when interacting with the token it's fine to leave as is!
Co-authored-by: alvarius <[email protected]>
…s report depending on internal store vs world
symbol: string; | ||
}; | ||
|
||
export function defineERC20Config({ namespace, name, symbol }: DefineERC20ConfigInput) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about defineERC20Module
since it returns module input?
and can we specify the return type here so we know we're always getting config-compatible module input
export function defineERC20Module(...): Module
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should save this file for doing the exports, not where the logic lives
so I would move this file to ts/defineERC20Module.ts
then this file becomes
export { defineERC20Module } from "../defineERC20Module";
No description provided.