Update to ethers v6 #137

dtbuchholz · 2024-04-18T16:24:39Z

Summary

Migrates all code to ethers v6
Adds Polygon Amoy, deprecates Mumbai

Closes #77.

Details

Primarily, refactoring for all code to use ethers v6, which required considerable testing to resolve nonce issues with tests. See the getAccounts method in the LT package for details—this must be used in all tests for ethers v6 to work with harhat when there are many txs in a row.
Also, added Polygon Amoy support. Note this also changed the chain names for Polygon to use polygon vs. matic.
Additional logic tweaks due to refactoring (will comment for awareness).

Note: node 16 has been dropped in hardhat & @tableland/evm, so expect that test to fail.

How it was tested

Updated all tests across all clients.

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 · 2024-04-19T01:23:13Z

packages/cli/package.json

-    "@tableland/node-helpers": "^0.0.2",
-    "@tableland/sdk": "^6.0.0",
+    "@tableland/node-helpers": "^1.0.0",
+    "@tableland/sdk": "^7.0.0",
    "@tableland/sqlparser": "^1.3.0",
    "@tableland/studio-cli": "^0.1.0-pre.3",


note: optionally, we need the studio CLI to get bumped before we publish this CLI (will comment on why down below).

dtbuchholz · 2024-04-19T01:23:27Z

packages/cli/src/cli.ts

@@ -44,8 +44,6 @@ export interface GlobalOptions {
  providerUrl: string;
  baseUrl: string;
  verbose: boolean;
-  ensProviderUrl?: string;


ENS has been dropped from the CLI

dtbuchholz · 2024-04-19T01:25:36Z

packages/cli/src/commands/controller.ts

@@ -82,9 +82,10 @@ export const builder: CommandBuilder<Record<string, unknown>, Options> = (
          });

          const reg = new Registry({ signer });
-          const res = await reg.setController({ tableName: name, controller });
+          const tx = await reg.setController({ tableName: name, controller });
+          const res = await helpers.getContractReceipt(tx);


ethers v6 will return a transaction response with a bigint type. thus, the JSON.stringify(...) down below errors out (with do not know how to serialized BigInt). we can choose to process the bigint -> string or number...but i chose to just use the method we already have (less data & includes the essentials).

dtbuchholz · 2024-04-19T01:27:39Z

packages/cli/src/commands/info.ts

-        ? err?.cause?.message
-        : err.message
-    );
+    if (err.message.match(/table name has wrong format/)) {


changed the logic to use the sqlparser and validated table names. to keep the errors consistent, I'm catching and using the same one we defined before.

dtbuchholz · 2024-04-19T01:29:06Z

packages/cli/src/commands/studio.ts

@@ -6,6 +6,7 @@ import type { GlobalOptions } from "../cli.js";
 export const command = "studio <sub>";
 export const desc = "Tableland Studio CLI commands";

+// @ts-expect-error polygon amoy vs mumbai is `temporarily` causing a type error


here's why the studio CLI should be published before this CLI, and then this CLI needs to update its deps before it gets published.

the studio CLI still has mumbai as a chain, so once it drops mumbai for amoy, then this TS directive is unneded

I think this makes sense.
To be explicit, the plan is:

publish the sdk, node-helpers, and local

update sdk dep and publish studio-cli

update studio-cli dep and publish this cli

Does that sound right? If so, let's publish a pre- version of the sdk, then open a pr for the studio-cli update, then update this PR with the new studio-cli version.

@joewagner cool. so do i need to change the SDK version in this PR to ...-pre.0? or, can we just publish the SDK "offline" as pre.0 and then proceed with what's outlined above?

Publishing pre versions "offline" is what I've done in the past.
I wrote a playbook in Notion, but it's basically cd into the packages/sdk directory, publish with npm (not lerna) and make sure to include --tag next

gotcha. but, my username doesn't have the ability to publish to npm, tho. maybe you could run that step for me?

oops, forgot joe's a bit OOO this week. @asutula or @carsonfarmer—if either of you have npm access, could you help publish a pre version of the SDK only? it'd be7.0.0-pre.0, and the process is described above.

(i need this to get the Studio changes going that use this PR, and i also want to get DIMO the new SDK w/ amoy so they can keep building.)

dtbuchholz · 2024-04-19T01:32:51Z

packages/cli/test/controller.test.ts

-const wallet = accounts[1];
-const provider = getDefaultProvider(TEST_PROVIDER_URL, { chainId: 31337 });
-const signer = wallet.connect(provider);
+const accounts = getAccounts(TEST_PROVIDER_URL);


when you review the LT getAccounts method, you'll see I've changed it to accept either and LT instance, or a URL. this is required because we must use a provider with custom settings—otherwise, it leads to a flakey "nonce to low" issue for all tests.

plus, this makes it a bit easier as compared to the previous logic, which had to be that way because of our custom port setups during tests. now, the custom ports are passed to the method as part of the URL.

dtbuchholz · 2024-04-19T01:34:50Z

packages/local/package.json

@@ -17,7 +17,7 @@
    "ethereum"
  ],
  "engines": {
-    "node": ">=14.0.0"
+    "node": ">=18.0.0"


bumped to node 18 b/c the new hardhat requires LTS / works with >=18

dtbuchholz · 2024-04-19T01:35:44Z

packages/local/src/util.ts

 * @returns An instance of a Tableland `Validator` with the correct `baseUrl`.
 */
-export const getAccounts = function (instance?: LocalTableland): Wallet[] {
+export const getAccounts = function (


here's the getAccounts logic change

dtbuchholz · 2024-04-19T01:37:00Z

packages/sdk/README.md

@@ -10,10 +10,13 @@



i'll update the README separately...i don't think much, if anything, has changed, but I'll take care of that when i update the docs site

dtbuchholz · 2024-04-19T01:38:19Z

packages/sdk/src/helpers/config.ts

@@ -2,7 +2,7 @@ import { type WaitableTransactionReceipt } from "../registry/utils.js";
 import { type FetchConfig } from "../validator/client/index.js";
 import { type PollingController } from "./await.js";
 import { type ChainName, getBaseUrl } from "./chains.js";
-import { type Signer, type ExternalProvider, getSigner } from "./ethers.js";


much of the following is where the core of the ethers v6 changes start. this old branch has a lot of the changes already mapped out, and i followed the majority of them: https://github.com/tablelandnetwork/js-tableland/tree/joe/bump-ethers

dtbuchholz · 2024-04-19T01:40:55Z

packages/sdk/src/helpers/ethers.ts

+ * @param signer A signer instance.
+ * @returns Current gas fee information for the network.
+ */
+export async function getFeeData(signer: Signer): Promise<FeeData> {


we had issues with doing anything in @tableland/evm without fetching live fee data. i've included the logic here to fetch data for amoy, and i figured doing it for polygon mainnet as well wouldn't hurt. if neither of those chains are provided, then it hits the generic ethers getFeeData method.

dtbuchholz · 2024-04-19T01:44:12Z

packages/sdk/src/helpers/ethers.ts

@@ -82,28 +139,49 @@ export interface MultiEventTransactionReceipt {
 *
 */
 export async function getContractReceipt(
-  tx: ContractTransaction
+  tx: ContractTransactionResponse


this is where i had to refactor a bit. a couple of notes:

event/log parsing is more difficult in v6. perhaps there's an easier way.

here, i created a new type (will explain below) that builds upon the types in the subscription logic where the index of a tableId is mapped to the event type.

in getContractReceipt, i use those types when i parse the args to get the index of a tableId

i also added support for setting a controller and transferring a table

dtbuchholz · 2024-04-19T01:44:52Z

packages/sdk/src/helpers/ethers.ts

+  // `_detectNetwork` here ensures this doesn't happen, but follow here for
+  // this issue getting resolved in the future:
+  // https://github.com/ethers-io/ethers.js/issues/4377#issuecomment-2023855491
+  await browserProvider._detectNetwork();


in case it ever comes up—this is a super annoying issue. see the link for details, but TL;DR it logs an error message every second until a connection is made

dtbuchholz · 2024-04-19T01:46:25Z

packages/sdk/src/helpers/ethers.ts

+ * @param options Optional settings for the provider. See ethersjs
+ * {@link JsonRpcApiProviderOptions} for more information.
+ */
+export function createSigner({


this is a new method, not really used anywhere. i think it's useful because pretty much every docs snippet starts with creating a pk signer. this just abstracts it away a little bit.

dtbuchholz · 2024-04-19T01:48:21Z

packages/sdk/src/helpers/subscribe.ts

+ * Possible events that can be emitted by the Registry as defined in the
+ * ITablelandTables interface.
+ */
+export type ContractEventNames =


here's where new types are defined for the event/log parsing i mentioned above. basically, I'm strongly typing the the registry's event names to silence some TS compiler complaints.

dtbuchholz · 2024-04-19T01:50:34Z

packages/sdk/src/helpers/subscribe.ts

+/**
+ * Mapping of all ITablelandTables contract events and `tableId` event index.
+ */
+export const contractEventsTableIdx: ContractEventTableIdMap = {


before, there was a single object contractEvents object (further below) that tracked events to their tableId and emit name.

with contractEventsTableIdx, it basically does the same but:

includes create table events

doesn't use the emit keyword
i suppose i could maybe combine them. anyways, the contractEventsTableIdx is used in the getContractReceipt above.

dtbuchholz · 2024-04-19T01:51:05Z

packages/sdk/src/helpers/subscribe.ts

@@ -260,7 +313,8 @@ export class TableEventBus {
        });
      };

-      contract.on(key, listener);
+      const topicFilter = contract.filters[key as SubscribableEventNames];


some additional logic i had to change for filtering events/logs in ethers v6

dtbuchholz · 2024-04-19T01:53:46Z

packages/sdk/src/validator/client/fetcher.ts

@@ -262,7 +262,7 @@ function fetcher<Paths>() {
                init: mergeRequestInit(defaultInit, init),
                fetch,
              })
-          )) as CreateFetch<M, Paths[P][M]>,
+          )) as unknown as CreateFetch<M, Paths[P][M]>,


nre TS was complaining if i didnt cast and then recast. i couldn't figure out why. but, if i changed it to CreateFetch to have a string value for either of the generics, the error went away. not a big deal tho.

dtbuchholz · 2024-04-19T01:54:23Z

packages/sdk/test/aliases.test.ts

-  const [, wallet] = getAccounts();
-  const provider = getDefaultProvider(TEST_PROVIDER_URL);
-  const signer = wallet.connect(provider);
+  const [, signer] = getAccounts(TEST_PROVIDER_URL);


just another callout to the updated getAccounts that returns a signer that we need use everywhere, avoiding nonce issues

dtbuchholz · 2024-04-19T01:57:06Z

packages/sdk/test/registry.test.ts

@@ -74,7 +70,7 @@ describe("registry", function () {
            err.message,
            // Actual hidden error:
            // reverted with custom error 'OwnerQueryForNonexistentToken()'
-            /cannot estimate gas; transaction may fail or may require manual gas limit.*/
+            /execution reverted (unknown custom error)*/


note: in case it's used elsewhere—the ethers v6 error message for onchain reverts has changed

dtbuchholz · 2024-04-19T02:02:09Z

packages/sdk/test/subscribe.test.ts

          match(from, /^0x[0-9a-fA-F]+$/);
          match(to, /^0x[0-9a-fA-F]+$/);
          strictEqual(tableId, eventTableId);
-          strictEqual(txn.event, "TransferTable");
-          match(txn.transactionHash, /^0x[0-9a-fA-F]+$/);
+          strictEqual(log.fragment.name, "TransferTable");


nre another random ethers v6 change that could be useful to know, if you're ever parsing events

dtbuchholz · 2024-04-19T02:03:20Z

packages/sdk/test/subscribe.test.ts

@@ -243,8 +238,16 @@ describe("subscribe", function () {

      await eventBus.addListener(`${tableName}`);
      const listeners = eventBus.listeners[tableIdentifier];
-
-      strictEqual(eventBus.contracts["31337"] instanceof Contract, true);
+      // TODO: understand why checking `instanceof Contract` doesn't work


note: i had this issue in the @tableland/evm package as well. although a contract is typed as a BaseContract in typechain, the compiler doesn't recognize it as matching the ethers Contract interface

dtbuchholz · 2024-04-19T02:10:20Z

@joewagner this is a hefty one! it includes both the ethers v6 and amoy changes.

it took forever to figure out that flakey nonce issue, but things finally work. note that we're dropping node 16 support, so that test doesn't matter.

I'm in the process of updating the studio with these changes. ideally, we:

publish the sdk, node-helpers, and local
publish the studio-cli
bump the studio-cli in this repo's cli package.
publish the cli package

dtbuchholz · 2024-04-19T02:45:29Z

packages/sdk/src/validator/client/validator.ts

note: i updated the docs file locally and then ran the SDK build:api command off of that, so this should match in the future once the docs site gets updated.

however, if you run the build process, it'll overwrite this file at packages/sdk/src/validator/client/validator.ts. if that's problematic, i can revert this change or push a quick docs change to main

…' for hardhat

dtbuchholz · 2024-04-19T23:54:32Z

(@joewagner rebased this on the latest changes from main, looks like that fixed the review workflow issue!)

packages/cli/src/utils.ts

OP Sepolia, FIL Calibration, and Hardhat all log an RPC error if gas estimation is attempted. Other testnets seems to work just fine.

joewagner · 2024-04-30T21:48:39Z

packages/sdk/src/helpers/ethers.ts

+  const chainIdNumber = typeof chainId === "bigint" ? Number(chainId) : chainId;
+  // Hardhat, Optimism Sepolia, & Filecoin Calibration will return an RPC error
+  return !(
+    chainIdNumber === 31337 ||


@dtb I'm wondering if we know for sure that chains with id 31337 always don't have these methods?

For background, there was a user in discord using that chainId for a private chain and they were using that chain id. Also there are some other local dev chains that use this id, e.g. https://chainlist.org/chain/31337

hm, yeah that's a good point; we can remove 31337. the main thing this method does is silence an error for The method "eth_maxPriorityFeePerGas" does not exist / is not available.', data: {…}}, which will get logged in the console of the Studio/web apps on those 3 chains. but, who cares if it gets logged with LT tbh.

some additional context: anvil and hardhat don't support the eth_maxPriorityFeePerGas method but do support eth_gasPrice. e.g., when you try to call the native ethers getFeeData on hardhat, the eth_maxPriorityFeePerGas method logs an error, but the tx still gets executed. aka we don't need to include 31337 because it'll still work fine.

edit: it's probs easier to just stick to removing 31337 from the filter and not do what's mentioned below. OP sepolia and FIL calibration networks work fine w/o custom gas prices.

~~also, we could adjust this method to destructure the gasPrice too. we just have to handle logic to avoid an error for Cannot send both gasPrice and maxFeePerGas params. e.g.:~~

// also get the gas price and conditionally pass it const { gasPrice, maxFeePerGas, maxPriorityFeePerGas } = await getFeeData( signer ); if (maxFeePerGas != null && maxPriorityFeePerGas != null) { opts.maxFeePerGas = maxFeePerGas; opts.maxPriorityFeePerGas = maxPriorityFeePerGas; } else if (gasPrice != null) { opts.gasPrice = gasPrice; }

~~to get the gas price, we'd have to call eth_gasPrice directly to avoid the logging error from above.~~

sticking to the removal of 31337 sounds good to me. If it's annoying in LT we can stop logging those messages in LT? (If that makes sense, we do it in another PR)

works for me!

joewagner · 2024-05-03T21:12:37Z

@dtbuchholz I just pushed a commit with some updated versions of the studio. I think we are gtg for Monday.

The only notable thing is that since we have two monorepos with a circular dependency, I had to do something a little hacky to enable us to publish everything simultaneously next week.
I had to temporarily move the sdk and local out of the monorepo and publish a next version of those so that the studio could properly npm install and npm run build. After that I published the next versions of studio without issue, which then allow us to npm install and npm run build this monorepo normally.
The only resulting weirdness is that I had to add two lint ignores and ts ignores to the local up.ts file. This is because in the monorepo there's no error, but when the directory gets moved out of the npm workspace there is an error.

tl;dr; we can either just leave them incase we do something like this again, or remove them once all the latest releases are published.

dtbuchholz · 2024-05-03T22:14:55Z

gotcha. i suppose if leaving those files doesn't impact anything negatively, then why not do so...just in case we have a major bump in the future, which I'm guessing will happen

dtbuchholz force-pushed the dtb/ethers-v6 branch from ab90f87 to 4eae71b Compare April 19, 2024 01:06

dtbuchholz commented Apr 19, 2024

View reviewed changes

dtbuchholz marked this pull request as ready for review April 19, 2024 02:08

dtbuchholz requested review from joewagner, carsonfarmer and sanderpick as code owners April 19, 2024 02:08

dtbuchholz force-pushed the dtb/ethers-v6 branch from 022cf20 to e7b4060 Compare April 19, 2024 02:42

dtbuchholz commented Apr 19, 2024

View reviewed changes

dtbuchholz added 13 commits April 19, 2024 16:44

chore: bump @tableland/evm

fbe62a7

test(sdk): update for ethers nonce issue

7e6d87d

feat(local): bump @tableland/sdk | polygon amoy | update 'getAccounts…

3e98c8f

…' for hardhat

feat(sdk): polygon amoy | new 'getAccounts' in tests

ec9aea4

feat(node-helpers): bump @tableland/sdk | use new 'getAccounts' in tests

e7c0b2a

fix(sdk): filter events for tableId logic

17dcd8c

feat(cli): polygon amoy | refactors for ethersv6

2299500

text(sdk|cli): c8 comments for coverage purposes

119a8f6

chore: bump root package lock | remove node 16 from gh action

7a1b54b

chore(cli): revert dep -> latest studio-cli version

2796e34

fix(sdk): move updated 'validator.ts' file to correct dir

5db4c2f

chore(repo): remove node 16, add 20 in review workflow

99dac22

fix(cli): remove uneccessary logging

d30895d

dtbuchholz force-pushed the dtb/ethers-v6 branch from 0958bd7 to d30895d Compare April 19, 2024 23:46

chore(repo): run prettier/lint

e44ea72

joewagner reviewed Apr 21, 2024

View reviewed changes

packages/cli/src/utils.ts Outdated Show resolved Hide resolved

dtbuchholz added 5 commits April 22, 2024 08:32

refactor(cli): use polygonscan URL over oklink

2e8a751

feat(sdk): export fee data helper method

384e481

chore(repo): bump ethers dep

9a057f5

chore(sdk): bump @tableland/evm

e4656db

fix(sdk): only estimate gas for valid networks

2c3b92b

OP Sepolia, FIL Calibration, and Hardhat all log an RPC error if gas estimation is attempted. Other testnets seems to work just fine.

joewagner reviewed Apr 30, 2024

View reviewed changes

dtbuchholz and others added 2 commits April 30, 2024 17:17

refactor(sdk): don't filter 31337 out for 'getFeeData'

cb642b8

Update versions with studio pre-releases

559d2ca

joewagner approved these changes May 3, 2024

View reviewed changes

dtbuchholz merged commit c1d3b2a into main May 3, 2024
4 checks passed

dtbuchholz deleted the dtb/ethers-v6 branch May 3, 2024 22:15

Update to ethers v6 #137

Update to ethers v6 #137

Conversation

dtbuchholz commented Apr 18, 2024 • edited Loading

Summary

Details

How it was tested

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtbuchholz Apr 19, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtbuchholz Apr 19, 2024 • 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

dtbuchholz Apr 19, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtbuchholz Apr 19, 2024 • 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

dtbuchholz commented Apr 19, 2024

dtbuchholz Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

dtbuchholz commented Apr 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtbuchholz Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joewagner commented May 3, 2024

dtbuchholz commented May 3, 2024

dtbuchholz commented Apr 18, 2024 •

edited

Loading

dtbuchholz Apr 19, 2024 •

edited

Loading

dtbuchholz Apr 19, 2024 •

edited

Loading

dtbuchholz Apr 19, 2024 •

edited

Loading

dtbuchholz Apr 19, 2024 •

edited

Loading

dtbuchholz Apr 19, 2024 •

edited

Loading

dtbuchholz Apr 30, 2024 •

edited

Loading