-
Notifications
You must be signed in to change notification settings - Fork 194
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): simplify access control to namespaces instead of routes #467
Merged
Merged
Changes from 20 commits
Commits
Show all changes
27 commits
Select commit
Hold shift + click to select a range
2e4d394
feat(world): add registerNamespace
alvrs 3148139
feat(world): use namespace for registerTable
alvrs 727dd07
refactor(world): replace all route based logic with namespace based l…
alvrs 989680c
test(world): update tests to use namespaces (wip)
alvrs cf1ccd4
feat(store): make error logs more expressive
alvrs 40245f9
test(world): fix testConstructor
alvrs a0e907c
fix(world): fix setMetadata
alvrs 7457f24
test(world): fix testRegisterSystem test
alvrs 1e7fea6
test(world): fix testRegisterTable
alvrs ba78b77
Merge branch 'main' into alvrs/namespaces
alvrs dcf6e57
chore(world): remove unused tables/schemas
alvrs 8f38145
chore(world): add TODO comment
alvrs 5173d5b
feat(world): add ResourceTable and prevent duplicate resource selectors
alvrs 4435185
feat(cli): update deploy-v2 and tablegen to namespaces
alvrs cb0e850
refactor(world): using ResourceSelector for bytes32
alvrs 79af020
refactor(world): using ResourceSelector for bytes32 in tests
alvrs 25cc03f
chore: update tablegen and gas-reports
alvrs 97f785f
chore: self-review
alvrs 5f57381
Merge branch 'main' into alvrs/namespaces
alvrs fd01e9c
chore: update codegen files in cli
alvrs 7cc5841
docs(world): remove redundant comment
alvrs 3d6396a
refactor(store): using TableId for uint256
alvrs fa48ffa
feat(store): change schema table id to cleartext id
alvrs 7bf2d0e
chore: update gas-report
alvrs 23f268c
chore: update comment
alvrs e2787e0
feat(store): set namespace store tables to mudstore
alvrs 941e72f
Merge branch 'main' into alvrs/namespaces
alvrs File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
does
abi.encodePacked
leave padding for an emptybytes16
?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.
also are we able to make library calls when building these constants? it would be nice to do like
and if not, maybe a uint type alias?
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.
yes,
bytes16
always takes 16 bytes inabi.encodePacked
, independent of its contentUnfortunately not - we're lucky
abi.encodePacked
is an exception that is considered "constant", we can't even do bitshifts in constantsLike
type TableId is uint256
? I think we could do that. Can you elaborate on why you'd prefer that over justuint256
?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.
I guess the type alias wouldn't matter if we can't call methods on that alias to help construct the ID at definition time. So it's not much better.
Though a type alias might be more useful when importing the constant and using the table ID in things like logging. If we pass around table IDs as
TableId
, then those reverts you have here could beThere 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.
The slightly annoying thing with a custom
TableId
type isuint256
can't be implicitly converted toTableId
, so we'd have to doTableId.wrap(tableId)
in all consuming libraries that call any Store function. For simplicity my preference would be to keep ituint256
.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.
Okay think I found a good compromise: with a library
TableId
andusing TableId for uint256
inStoreCore
we can dotableId.toString()
, without the overhead of a custom typeThere 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.
Would this affect all uints? Wondering if we should be more explicit like castToString.
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.
this means inside of
StoreCore
you could call thetoString
method on alluint256
, yes. But it's only local toStoreCore
and as long as we don't do that it should not be a problem (I don't see any other reason to calltoString
here). We can also just useTableId.toString
and removeusing for
if you think it's an issue.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.
I've just personally used
toString
onuint
specifically to translate e.g.123
into"123"
. Same is true for a couple of common Solidity libraries: https://github.com/transmissions11/solmate/blob/main/src/utils/LibString.sol#L4-L8So I'm just a little wary of overlapping naming. I'll personally need to make a mental leap to remind myself that this is more of a cast operation.
But not blocking, since this is internal!