feat(cli): refactor tablegen and add more advanced features #437

dk1a · 2023-02-24T23:29:17Z

Pretty much rewrote the whole thing.
StoreUserConfig is a good summary of what's going on.
All tables in store are now autogenerated, check them out.
I didn't want to mess with world too much, so made just RouteAccess autogenerated (it's interesting because it has 2 keys with different types, and uses storeArgument flag)

#438 has a summary

…validation

holic · 2023-02-25T14:51:47Z

packages/store/src/tables/Route.sol

 struct Route {
  address addr;
  bytes4 selector;
  uint8 executionMode;
 }

-library Route_ {
+library RouteTable {


I think I missed the naming conversation but did ya'll consider RouteStore as a name option?

I think it'll be confusing when a table that writes to Store is also called Store. We already have World, which is Store. Wouldn't want everything to be a Store

My thought is that users will mostly be interacting with these autogen files, not the underlying ones (i.e. Store). But I understand if this creates confusion on our end.

"Table" just feels like an overloaded term. Folks with SQL experience will have preconceived notions about what it does/should do, and folks without SQL experience may be a little confused by it the term "table". We have a chance to shed this here.

(All that said, I don't feel super strongly, just wanting to stress test our name choices as it'll be hard to change later)

I like the SQL analogy (I have SQL experience, although I'm obviously not a great example).
We even have hooks/indexers. And a planned postgres integration. In solidity it's just a limited key/value store of course.
But I'm starting to think of how foreign keys would fit in here

to circle back on this: we all chatted in discord and ended up with removing the suffix altogether on libraries, and adding "data" suffix to structs, allowing users to choose their naming scheme

holic · 2023-02-27T22:13:11Z

packages/cli/src/utils/errors.ts

-  }
+export class NotInsideProjectError extends Error {
+  name = "NotInsideProjectError";
+  message = "You are not inside a MUD project";
 }


small thing: I would probably add

export class MudCliError extends Error {}

then

export class NotInsideProjectError extends MudCliError { ... }

just so downstream folks can easily catch all CLI errors in one conditional

(I am thinking we might eventually have a centralized MudError but a cli-specific one in the meantime is still handy)

That said, I'm not sure how these interact with Zod error classes.

just so downstream folks can easily catch all CLI errors in one conditional

There aren't really supposed to be downstream folks rn, the renderer is cli-only currently, and everything is handled by logError.
When I do make it standalone that sounds like a good thing to add though. Zod errors can probably be wrapped to also be identifiable somehow

holic · 2023-02-27T22:19:02Z

packages/cli/src/utils/format.ts

+    console.log(chalk.yellow(`Error during output formatting: ${message}`));
+    return content;
+  }
+}


Not blocking but we can also call out to forge fmt and use the project's local foundry.toml (in the same way that we use it for src etc. dirs) for formatting config

cat File.sol | forge fmt --raw -

This method isn't really file-related, it's used at the render stage, so it's not aware of the filesystem at all, and when renderer is standalone it could even be used without files at all.
Also since it's all js using prettier is just more convenient.
I think if people want custom formatting they can do their own fmt, this func is just for new mudders who don't have that set up yet, so they don't see super ugly files

alvrs

Another very impressive PR! Just a couple smaller nits / questions

packages/cli/src/utils/errors.ts

packages/cli/src/config/loadConfig.ts

packages/cli/src/utils/errors.ts

packages/cli/src/config/loadConfig.ts

packages/cli/src/config/loadStoreConfig.ts

packages/cli/src/render-table/index.ts

alvrs · 2023-02-27T23:22:05Z

packages/cli/src/config/loadStoreConfig.ts

+
+const FullTable = z
+  .object({
+    route: OrdinaryRoute.default("/tables"),


looks like this can't be empty correct? I think we should allow registering tables at the root level of a World (eg. /RootTable), which would require passing the empty root route here, however this wouldn't work as nicely with using the same route as output path for the generated file

Correct, it's the one case that messes up route->folder.
Do we really need root tables?

I think it might be useful, if only to allow users to minimize route lengths. Just feels like we shouldn't prevent a setup that uses tables at the root. We could even just put the auto-generated file into the "base directory" (right now foundry's src config - but we could add a config field to allow overriding the table base directory).

Not blocking for this PR though, let's discuss and change in a follow up if needed

Just allowing root routes is easy actually - replace validateRoute with validateBaseRoute in zod schema.
Having an extra config option for baseDirectory is also pretty trivial, although baseRoute+baseDirectory feels like a bit much.

Btw I don't bundle systems and tables together here. Systems are writers, and for systems that have access to everything (i.e. use delegatecall) I agree root is the best place.

Problems I have with root tables:

Encourages flat setup. It's intuitive for small projects, but projects that need MUD aren't really small imo. No root simplifies the switch from "/tables" to "/skills", "/items" etc.

Tables no longer have access groups. If you want to give access to several at once, you'd give root route. You shouldn't give out root access.

Unlike systems, root tables don't signify a root permission. They don't have to exist

Rather than root routes I think it'd be more useful to consider shorthands/config reorgs for setting the same route for multiple tables/systems

My current folders (they would more or less become routes in v2, and this sorta setup is what I'm optimizing for):

On the other hand, if route and folder path are completely separated, it feels like my problems with root tables kinda evaporate. And maybe we should have route: "" by default, but path: "/tables" by default, and have no significant relationship between the 2

Encourages flat setup. It's intuitive for small projects, but projects that need MUD aren't really small imo. No root simplifies the switch from "/tables" to "/skills", "/items" etc.

I think with v2 there are a lot of good reasons for small projects to use MUD too (eg default compatibility with indexers and network stack), so my preference would be to make all project configurations possible, including simple/flat ones.

I still agree with making more complex / nested project structures like your example easy to achieve too. After thinking about it more I think separating on-chan route and filesystem outputPath like you proposed makes the most sense, because it allows for any configuration of routes and filesystem structure. This also makes the id computation consistent with systems, where it's always keccak256(baseRoute/route/SystemName) with route being "" by default. (Also for systems route is independent from the file system path too because they are not auto-generated/)

While working on the system deploy script I am realizing it would be most intuitive if route allows users to completely override the route, without a default /SystemName or /TableName appended at the end. So basically if route is omitted the default route should be baseRoute/SystemOrTableName, while if the route key is present it should be baseRoute/route. Otherwise we might prevent some configurations where the route has to be some explicit value that's not the contract name.

While working on the system deploy script I am realizing it would be most intuitive if route allows users to completely override the route, without a default /SystemName or /TableName appended at the end. So basically if route is omitted the default route should be baseRoute/SystemOrTableName, while if the route key is present it should be baseRoute/route. Otherwise we might prevent some configurations where the route has to be some explicit value that's not the contract name.

Sounds good! I can change that in store config in one of the upcoming PRs

Co-authored-by: alvarius <[email protected]>

dk1a added 4 commits February 25, 2023 02:25

refactor(cli): rename typeWithLocation

27f08d4

feat(cli): add routes, schemaMode (unimplemented) to config, improve …

4fa8b9f

…validation

feat(cli): implement schemaMode flag

76b7624

refactor(store): use new tablegen in store

3ed47b2

dk1a requested review from alvrs and holic as code owners February 24, 2023 23:29

dk1a added 2 commits February 25, 2023 02:51

refactor(cli): improve some rendering utils

f488e96

feat(cli): add support for singleton tables

b6c1849

dk1a mentioned this pull request Feb 25, 2023

Tracker: v2 tablegen todos #438

Open

47 tasks

holic reviewed Feb 25, 2023

View reviewed changes

dk1a added 20 commits February 25, 2023 23:15

feat(cli): autoformat tablegen output

a9da26c

refactor(cli): settle on pure ESM config, add .ts support

5bae806

refactor(cli): use zod for StoreUserConfig validation

5184bff

chore(cli): remove outdated comment about hardhat

28a383f

build(cli): clean up dependencies

f933bc1

refactor(cli): improve errors

89e9802

refactor(cli): add validation error customization

5bffa3c

feat(cli): add component mode

124363c

fix(cli): add NotESMConfigError and fix esnext resolution issues

3b06990

build(cli): switch to nodenext resolution

ae991bc

build(cli): add dev script for tsup --watch

a7e4a60

refactor(cli): extract common zod schemas

4d94b84

feat(cli): add component mode shorthand

69a3ab3

fix(cli): report and continue for errors during formatting

abb1dd2

fix(cli): fix commas for combined singleton and schema modes

7752bc5

refactor(cli): update some old conditionals with better syntax

20e2c83

feat(cli): improve struct and library naming

e7ba345

refactor(cli): improve config arguments

0332857

feat(cli): support no struct for multicolumn tables

d4f5464

refactor(cli): rename renderSchema to renderTable

5a84751

dk1a added 12 commits February 27, 2023 00:25

feat(cli): improve code structure for renderTable

84e0f52

fix(cli): make dir if absent

8a73ae9

feat(cli): generate push for dynamic fields

c557d4c

feat(store): make all tables autogenerated

62a82e5

feat(cli): generate deleteRecord

0d9e807

feat(cli): add primary key typing

5a4ab13

refactor(store): use new tablegen

c375c3a

fix(cli): add missed bool cast

3330d93

fix(cli): always render deleteRecord

829ecb1

feat(cli): add storeArgument option

f4eb23b

refactor(store): update store tables

8bf6a4b

feat(world): use tablegen for RouteAccess

c53bf9b

dk1a changed the title ~~feat(cli): add routes and schemaMode to tablegen, add minor improvements~~ feat(cli): refactor tablegen and add features Feb 27, 2023

fix(cli): missing bracket for int typecast

a1cdd5c

holic reviewed Feb 27, 2023

View reviewed changes

alvrs reviewed Feb 27, 2023

View reviewed changes

dk1a and others added 4 commits February 28, 2023 03:08

chore(cli): change route doc comment

d4c3d7d

Co-authored-by: alvarius <[email protected]>

chore(cli): fix typo 1

2012a57

Co-authored-by: alvarius <[email protected]>

chore(cli): fix typo 2

9c380c3

Co-authored-by: alvarius <[email protected]>

chore(cli): remove outdated comment

e88730b

alvrs approved these changes Feb 28, 2023

View reviewed changes

alvrs changed the title ~~feat(cli): refactor tablegen and add features~~ feat(cli): refactor tablegen and add more advanced features Feb 28, 2023

alvrs merged commit 4777719 into v2 Feb 28, 2023

dk1a deleted the dk1a/v2-tablegen-2 branch May 9, 2023 16:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cli): refactor tablegen and add more advanced features #437

feat(cli): refactor tablegen and add more advanced features #437

dk1a commented Feb 24, 2023 •

edited

Loading

holic Feb 25, 2023

dk1a Feb 25, 2023

holic Feb 25, 2023 •

edited

Loading

dk1a Feb 25, 2023

holic Feb 27, 2023 •

edited

Loading

holic Feb 27, 2023

holic Feb 27, 2023

dk1a Feb 27, 2023

holic Feb 27, 2023 •

edited

Loading

dk1a Feb 27, 2023

alvrs left a comment

alvrs Feb 27, 2023

dk1a Feb 28, 2023

alvrs Feb 28, 2023 •

edited

Loading

dk1a Feb 28, 2023

dk1a Feb 28, 2023

alvrs Feb 28, 2023

alvrs Feb 28, 2023

dk1a Feb 28, 2023

feat(cli): refactor tablegen and add more advanced features #437

feat(cli): refactor tablegen and add more advanced features #437

Conversation

dk1a commented Feb 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holic Feb 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holic Feb 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holic Feb 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvrs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvrs Feb 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dk1a commented Feb 24, 2023 •

edited

Loading

holic Feb 25, 2023 •

edited

Loading

holic Feb 27, 2023 •

edited

Loading

holic Feb 27, 2023 •

edited

Loading

alvrs Feb 28, 2023 •

edited

Loading