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

fix: mumbai failing to due gas limit issues #99

Merged
merged 1 commit into from
Nov 21, 2023
Merged

Conversation

dtbuchholz
Copy link
Contributor

@dtbuchholz dtbuchholz commented Nov 21, 2023

Summary

This fixes an issue where Mumbai txs were always failing for both creates and writes. It bumps the gas limit up by 20% for all Polygon chains. The CLI package has been updated with a patch bump to incorporate the new SDK version's patch. Also, small package.json updates were made for GH repo links in all packages. Closes #98.

Details

It affects all state altering methods. This includes create and mutate methods for both single and batch statements, setting/locking controllers, and transfers.

How it was tested

You can see a test create and write tx here, where the gas limit leaves about 20% of extra room:

Prior to this, it'd fail with a 99% gas usage, like here.

The controller and transfer methods don't necessarily need this logic and pass without needing to introduce the gas limit bump. These are passing transactions from the existing logic, before introducing the 20% bump:

However, you can see the transfer was at 99%, and controller was at 87% of the gas limit. Thus, it makes sense to also implement the gas limit bump for them as a margin of safety. Here are changes with the 20% bump included:

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 Nov 21, 2023

@joewagner question on the package version bumps. since everything uses the @tableland/sdk, is it a best practice to always publish new versions for cli/local/node-helpers if the SDK gets a new release?

i know the CLI always needs a bump to incorporate SDK changes. but something like node-helpers only uses the SDK as a dev dep. i figured it's ideal to keep things in sync but wanted to double check since node-helpers was previously using 5.0.0 whereas the CLI/local were using 5.1.0. wasnt sure if that was intentional or not.

@joewagner
Copy link
Contributor

@joewagner question on the package version bumps. since everything uses the @tableland/sdk, is it a best practice to always publish new versions for cli/local/node-helpers if the SDK gets a new release?

Not necessarily. The basic rule is, if you want a new version of a package to be published on npm you will need to update the version for that package.

Here are two examples that demonstrate some more complicated scenarios:

  1. We created a new feature in the sdk and want that to be available on npm. In this case we would increment the sdk minor version. cli and local don't have anything new to be published on npm, so they are not changed.
  2. There is a fix for a bug in the sdk that makes it's way downstream and causes a bug in local but not in cli. In this example we would increment the sdk and local patch versions.

Nuances of these two examples:

  • In example 1 the dependencies in local and cli don't need any change since they are ^x.x.x which means that users of our packages can have the flexibility to use the combinations of versions they want to use
  • In example 2 the local dependencies entry for @tableland/sdk should to be updated to whatever the new sdk version is... Obviously still using the ^ syntax.

i know the CLI always needs a bump to incorporate SDK changes. but something like node-helpers only uses the SDK as a dev dep. i figured it's ideal to keep things in sync but wanted to double check since node-helpers was previously using 5.0.0 whereas the CLI/local were using 5.1.0. wasnt sure if that was intentional or not.

The CLI does not always need a bump, but if you want to publish a new version of the CLI (because of a bug fix, or added feature) you do need to bump the version.

As an example of a time bumping a version would not make sense, imagine there is a bug in the sdk's implementation of exec. The CLI never uses exec, and there isn't any need to publish a new version of it.

It's all potentially a little complicated, does that answer your question?

Copy link
Contributor

@joewagner joewagner left a comment

Choose a reason for hiding this comment

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

@dtbuchholz I put some specifics on my thoughts inline. Happy chat in voice on this, it's potentially a bit confusing (and open to opinion).

packages/local/package.json Outdated Show resolved Hide resolved
packages/node-helpers/package.json Outdated Show resolved Hide resolved
packages/cli/package.json Show resolved Hide resolved
This fixes an issue where mumbai txs were always failing for both creates and writes. It bumps the gas limit up by 20%, affecting the registry create, write, transfer, and controller set/lock methods. You can see a test create and write tx here, where the gas limit leaves about 20% of extra room: https://mumbai.polygonscan.com/tx/0xd36a73256b38b866ebda65dff619174145768f35a2cb179f2ebc0f3832344bbc, https://mumbai.polygonscan.com/tx/0x236fa88bb9034629f9f4ea8e23a28ddd32bc6f13bb11ff1c602051e4689adf92
@dtbuchholz
Copy link
Contributor Author

@joewagner ah yes, that all makes sense. thanks for the explainers.

okay, i've reverted the version & dep bumps for local/node-helpers; only the SDK and CLI have been changed there.

Copy link
Contributor

@joewagner joewagner left a comment

Choose a reason for hiding this comment

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

💪

@dtbuchholz dtbuchholz merged commit 9f2b383 into main Nov 21, 2023
5 checks passed
@dtbuchholz dtbuchholz deleted the dtb/fix-polygon branch November 21, 2023 23:54
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.

[TABJS-19] Fix polygon mumbai gas limit issue causing failed txs
2 participants