Skip to content
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

refactor(network): change base uri, removing tables endpoint #456

Closed
wants to merge 2 commits into from

Conversation

dtbuchholz
Copy link
Contributor

Summary

The baseURIs include the tables/{chainId} endpoint, which shouldn't really be used. Instead, it should just be the true /api/v1 endpoint, attached to the correct gateway.

Details

See network.ts and baseURIs.

How it was tested

N/A

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@dtbuchholz
Copy link
Contributor Author

dtbuchholz commented Oct 28, 2023

Since this PR makes a tweak to network.ts, I figured I'd call the following out in case there's a change we want to make with TablelandNetworkConfig types, described here. TL;DR—the TS compiler doesn't always like the string | number type when we use evm-tableland imports in the SDK, which was introduced when we added validatorPollingTimeouts. Thus, a @ts-expect-error is needed to resolve the issue...not a big deal but just noting the behavior.

@dtbuchholz
Copy link
Contributor Author

dtbuchholz commented Oct 28, 2023

Also, @brunocalza—do we really need the optimism-goerli-staging definition in these exports? That config info is only really relevant to and is already configured on the validator, right? Or, do we use @tableland/evm for internal testing purposes somewhere, too, and need it to exist in this package?

edit: I suppose we do need it because we use that type def in the hardhat config, which we obv need in order to deploy to the staging network.

@dtbuchholz
Copy link
Contributor Author

@joewagner I came across the changesets tool while making a PR in D1's ORM repo. It's a pretty nice experience because you just run npx changeset on your feature branch, which prompts you for major/minor/patch and a description of changes—this gets written to a .changeset/<file.md>. Then, when you merge the branch, it'll automatically handle publishing to npm, tags, and can update the package.json's version, too.

If you think it's worthwhile, I can try it out on this repo since the publishing action is broken, anyways. It's also supposed to be nice for monorepos, but I'm not sure if we need that with our lerna setup for the SDK/CLI/LT.

@brunocalza
Copy link
Contributor

Also, @brunocalza—do we really need the optimism-goerli-staging definition in these exports? That config info is only really relevant to and is already configured on the validator, right? Or, do we use @tableland/evm for internal testing purposes somewhere, too, and need it to exist in this package?

edit: I suppose we do need it because we use that type def in the hardhat config, which we obv need in order to deploy to the staging network.

yeah, I guess you're right, we need it for deploying the contract

@joewagner
Copy link
Contributor

@joewagner I came across the changesets tool while making a PR in D1's ORM repo. It's a pretty nice experience because you just run npx changeset on your feature branch, which prompts you for major/minor/patch and a description of changes—this gets written to a .changeset/<file.md>. Then, when you merge the branch, it'll automatically handle publishing to npm, tags, and can update the package.json's version, too.

If you think it's worthwhile, I can try it out on this repo since the publishing action is broken, anyways. It's also supposed to be nice for monorepos, but I'm not sure if we need that with our lerna setup for the SDK/CLI/LT.

I'm not sure, but I think we tried changesets in the past and didn't end up going with it. @carsonfarmer would remember better than me.
I'd prefer to just fix the publish action instead of introducing more tooling. We publish this so infrequently that I've neglected to spend any time trying to fix the publish action. If you want to work on that, feel free. In the mean time let's publish this manually when its ready.

@@ -43,26 +43,25 @@ export const proxies: TablelandNetworkConfig = {
"local-tableland": "0xe7f1725e7734ce288f8367e1bb143e90bb3f0512",
};

const homesteadURI = "https://tableland.network/api/v1/tables/1/";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The chain Ids are used in the ERC721a tokenURI method. It simply appends the token / table Id to the baseUri. Wouldn't this change break that functionality?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sanderpick ah, i see. the use case i was considering this for was to be able to set the baseURI via a hardhat script that only uses evm-tableland, so it'd be nice to be able to import the baseURIs from network.ts. but, i think the TablelandDeployments baseURIs can help there, and instead, the SDK getBaseUrl can provide the correct URL info. i'll just close this out.

and @joewagner sgtm, will tal in the future with #414

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if all you need is the host name, you could still use these URLs... just pull the host out. /api/v1 will be the same for them all.

@dtbuchholz dtbuchholz closed this Oct 31, 2023
@dtbuchholz dtbuchholz deleted the dtb/adjust-networks branch October 31, 2023 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants