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

remove old v0 errors from js sdk. provide error details to the user #385

Merged
merged 10 commits into from
Feb 26, 2024

Conversation

glitch003
Copy link
Collaborator

@glitch003 glitch003 commented Feb 22, 2024

Users often get the error "There was an error getting the signing shares from the nodes". We should aim to include any details we have from the node, and not just return this opaque error. IMO the "After" message below is a lot better - it clearly indicates that there was a timeout.

We can still improve this a lot, but this is just a quick fix to ensure that the user can at least see what's coming out of the node. In reality, this timeout error should be a specific error code, etc, but that might be something we need to fix on the node side (aka, the node isn't returning a proper error code in this case, and it's returning some unknown error, but at least that error has details that help with debugging)

Before:

"error": {
      "message": "There was an error getting the signing shares from the nodes.",
      "errorKind": "UnknownError",
      "errorCode": "unknown_error"
    }

After:

"error": {
      "message": "There was an error getting the signing shares from the nodes",
      "errorCode": "unknown_error",
      "errorKind": "Timeout",
      "status": 500,
      "details": [
        "timeout limit reached timeout limit: 31000ms"
      ],
      "requestId": "a3073ee78572a"
    }

Or, in the case where we just didn't get enough shares:

"error": {
      "message": "The total number of valid signatures shares 2 does not meet the threshold of 4",
      "errorCode": "no_valid_shares",
      "errorKind": "Unexpected",
      "requestId": "d44599bd20edf"
    }

@glitch003 glitch003 marked this pull request as ready for review February 23, 2024 02:23
rahul-ramesh
rahul-ramesh previously approved these changes Feb 23, 2024
packages/core/src/lib/lit-core.ts Outdated Show resolved Hide resolved
@@ -528,7 +528,7 @@ async function buildFunc() {
.map((item) => item.replace('apps/', ''))
.join(',');

const command = `yarn nx run-many --target=build --exclude=${ignoreList}`;
const command = `yarn nx run-many --target=build --exclude=${ignoreList} --parallel=false`;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not related to this PR. Can we remove?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it's the only way i can get it to build consistently :(

anson's optimise/1 branch also fixes the build for me. so my plan is to remove --parallel=false when his optimise/1 branch lands. if we merge optimise/1 first then we could remove --parallel=false now.

packages/misc/src/lib/misc.ts Outdated Show resolved Hide resolved
@glitch003 glitch003 marked this pull request as draft February 24, 2024 02:52
@glitch003 glitch003 marked this pull request as ready for review February 24, 2024 04:23
@Ansonhkg Ansonhkg changed the base branch from master to staging/3.2.2 February 26, 2024 13:10
@Ansonhkg Ansonhkg mentioned this pull request Feb 26, 2024
5 tasks
@Ansonhkg Ansonhkg merged commit 5464279 into staging/3.2.2 Feb 26, 2024
1 of 4 checks passed
@Ansonhkg Ansonhkg deleted the feature/better-error-handling branch February 26, 2024 13:19
Ansonhkg added a commit that referenced this pull request Feb 28, 2024
* init patch

* Feature/lit 2570 js sdk fix build (#387)

* feat: make capacityTokenId optional

* chore: add test

* fix: Conditionally include nft_id

* fix: replace nx executor to @nrwl/js:tsc

* fix: replace nx executor

* feat: migrated nx to 15.9.0

* feat: migration nx from 15.9.0 -> 16.10.0

* feat: migrate nx to 17.3.0

* fix: yarn:test:unit command

* chore: log cc nft token info

* fix: undefined typings in package.json

* remove old v0 errors from js sdk.  provide error details to the user (#385)

* remove old v0 errors from js sdk.  provide error details to the user

* building now seems to consistently work

* refactored error handling to make it cleaner

* Remove unused function

* okay also added requestId in a few places

* ran the linter

* fix: replace nx executor

* feat: add gitignore

---------

Co-authored-by: Ansonhkg <ansonox@gmail.com>

* feat: cache enabled

* chore: update .gitignore

* chore: enable parallel

* feat: make capacityTokenId optional (#384)

* feat: make capacityTokenId optional

* chore: add test

* fix: Conditionally include nft_id

* Update yarn.lock to match package.json changes

* fix: ReferenceError: crypto is not defined

* fix: ignore nx cache directory on linting

* fix: ignore tools directory on linting

* fix: linting

* fix: '
ReferenceError: require is not defined in ES module scope, you can use import instead'

* fix: lint

* [Release] 3.2.2

* chore: move to manual tests

---------

Co-authored-by: Chris Cassano <1285652+glitch003@users.noreply.github.com>
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