-
Notifications
You must be signed in to change notification settings - Fork 1
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
fix: filter out optimism-goerli-staging
from chains
#59
Conversation
@joewagner i made a small adjustment to the SDK to fix a bug that was uncovered during ETHOnline while someone was using the Studio. it removes |
packages/cli/src/commands/write.ts
Outdated
name | ||
); | ||
const { tableId } = | ||
await globalThis.sqlparser.validateTableName(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.
Looks like the prettier settings are changed. Is this something to do with the deleted package lock?
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.
yeah, looks like it. i dropped that commit, and the formatting went back to normal. i'll just ignore any of the lerna-related things i included here...it might not be the root cause, anyways, after some additional testing...
@dtbuchholz this looks good. We might as well put the sdk at an official release number. Looks like something got messed up during the last publish and npm has the sdk's |
edacfbf
to
9152f4e
Compare
@joewagner i bumped the SDK versions in the cli & local packages to 4.5.3. but since it doesn't exist, the tests etc obviously fail here. is that fine, or should i move the dep bump to a separate PR? edit: i'll also have to bump the package/sdk's package.json version to 4.5.3 too, right? or does the publish GH action somehow handle that? (this change is not included yet, but i'll wait for your input above before i do anything there.) |
9152f4e
to
210a5b6
Compare
You should be able to set the version to |
@dtbuchholz I accidentally edited your comment instead of replying... |
@joewagner no worries...that's such a dumb feature lol
i'm down to take a stab! i'll ping you on discord since i know timezones are a bit tough rn |
Summary
If you try to create a table on OP Goerli, it actually uses the
optimism-goerli-staging
network instead ofoptimism-goerli
. This filters out the "staging" network and adds tests accordingly. Also, I squeezed in a dep bump for@tableland/sdk
. Closes #58.Details
There's a simple one liner added to
helpers/chains.ts
:This is then used in the
supportedChains
object, which gets exported and is used in clients like the CLI. Note that the CLI had to implement similar logic to what's described above. The change in the SDK will allow us to remove that check in the CLI, but it's not required at the moment; everything should still work as expected in the CLI.Wrt to the
@tableland/sdk@^4.5.3-dev.0
dep bump, I also deletedpackage-lock.json
and reinstalled the overall deps in root. It seems that lerna was having issues with symlinks, and deleting the lock file & updating the packages seemed to fix it. I think lerna might require that each package is using the latest version—e.g., inpackages/cli
, if I don't use4.5.3-dev.0
but use any earlier SDK version, I cannot set up a symlink successfully.How it was tested
Added a test in
chains.test.ts
to make sure thatgetChainId("optimism-goerli-staging")
will throw an error forcannot use unsupported chain
.I also set up a symlink to the Studio, and I successfully created a table on the correct OP Goerli network.
Checklist: