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.

Sign up for GitHub

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

Jump to bottom

Feature/lit 3536 cleanup shiva changes #521

Merged
merged 18 commits into from
Jul 25, 2024
Merged

Feature/lit 3536 cleanup shiva changes #521

merged 18 commits into from
Jul 25, 2024

Conversation

joshLong145
Copy link

@joshLong145 joshLong145 commented Jul 3, 2024 •
edited
Loading

Description

Addresses comments from PR: #515 around Shiva Client changes.

  • Fixes error handling and now throws over using Promise.reject
  • Refactors polling to no longer double check request status
  • Adds ShivaError as a convince method for transforming error results into an Error object type
  • Swaps dotenv for nx dotenv loader

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Ran tests against shiva to assure compatibility

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my 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

@joshLong145 joshLong145 marked this pull request as ready for review July 4, 2024 14:42
@joshLong145 joshLong145 requested a review from Ansonhkg as a code owner July 4, 2024 14:42
Ansonhkg
Ansonhkg approved these changes Jul 4, 2024
View reviewed changes
Copy link
Collaborator

@Ansonhkg Ansonhkg left a comment

Choose a reason for hiding this comment

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

Nice & better error handling!

local-tests/setup/shiva-client.ts Outdated Show resolved Hide resolved
@Ansonhkg Ansonhkg self-requested a review July 4, 2024 14:54
Ansonhkg
Ansonhkg requested changes Jul 4, 2024
View reviewed changes
Copy link
Collaborator

@Ansonhkg Ansonhkg left a comment

Choose a reason for hiding this comment

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

Need to fix CI build issue:

/opt/hostedtoolcache/node/20.15.0/x64/bin/node: .env: not found

@joshLong145
Copy link
Author

After re running e2e tests on other branches which previously passed it looks like our contract wrties are under what the current chain price is: https://github.com/LIT-Protocol/js-sdk/actions/runs/9751194755/job/27055335469?pr=516

So i do not think this pr is cause of failure.
@Ansonhkg @MaximusHaximus

@joshLong145 joshLong145 force-pushed the feature/lit-3536-cleanup-shiva-changes branch from 16b2877 to 06d03c2 Compare July 8, 2024 13:43
@joshLong145 joshLong145 changed the base branch from master to feature/lit-3549-js-sdk-add-datiltest-support July 8, 2024 15:39
@Ansonhkg Ansonhkg requested review from jtary and FedericoAmura July 8, 2024 17:21
@joshLong145 joshLong145 requested a review from Ansonhkg July 10, 2024 12:45
@joshLong145
Copy link
Author

@Ansonhkg @MaximusHaximus
Can we merge this?

MaximusHaximus
MaximusHaximus requested changes Jul 11, 2024
View reviewed changes
Copy link
Contributor

@MaximusHaximus MaximusHaximus left a comment

Choose a reason for hiding this comment

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

👍 Nice work, this is looking good. Just a few things I noticed added in-line

local-tests/setup/shiva-client.ts Show resolved Hide resolved
local-tests/setup/shiva-client.ts Outdated Show resolved Hide resolved
local-tests/setup/shiva-client.ts Outdated Show resolved Hide resolved
local-tests/setup/shiva-client.ts Outdated Show resolved Hide resolved
Ansonhkg and others added 2 commits July 11, 2024 22:18
Merge branch 'feature/lit-3549-js-sdk-add-datiltest-support' of githu…
2187722 
…b.com:LIT-Protocol/js-sdk into feature/lit-3536-cleanup-shiva-changes
@joshLong145 joshLong145 force-pushed the feature/lit-3536-cleanup-shiva-changes branch from d7d3fb8 to 2187722 Compare July 12, 2024 18:32
Base automatically changed from feature/lit-3549-js-sdk-add-datiltest-support to master July 15, 2024 15:04
Ansonhkg
Ansonhkg requested changes Jul 15, 2024
View reviewed changes
.env.sample Outdated Show resolved Hide resolved
@Ansonhkg
Update .env.sample
4f3c4a9 
Co-authored-by: Anson <[email protected]>
Signed-off-by: Bean <[email protected]>
MaximusHaximus
MaximusHaximus requested changes Jul 15, 2024
View reviewed changes
Copy link
Contributor

@MaximusHaximus MaximusHaximus left a comment

Choose a reason for hiding this comment

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

Almost thrre I think -- I added a few things in in-line; several are just the same comment/question in different functions.

local-tests/setup/shiva-client.ts Outdated Show resolved Hide resolved
local-tests/setup/shiva-client.ts Outdated Show resolved Hide resolved
local-tests/setup/shiva-client.ts Outdated Show resolved Hide resolved
local-tests/setup/shiva-client.ts Show resolved Hide resolved
local-tests/setup/shiva-client.ts Outdated Show resolved Hide resolved
local-tests/setup/shiva-client.ts Outdated Show resolved Hide resolved
Josh Long added 2 commits July 16, 2024 11:16
@joshLong145
Copy link
Author

@MaximusHaximus
Thanks for the review! I had a pending commit that is now pushed that addresses the comments.

@Ansonhkg Ansonhkg added 🐞 Bug Fix Something isn't working ✨ Enhancement New feature or request labels Jul 24, 2024
Ansonhkg
Ansonhkg approved these changes Jul 24, 2024
View reviewed changes
Copy link
Collaborator

@Ansonhkg Ansonhkg left a comment

Choose a reason for hiding this comment

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

I think this is good to go!

@joshLong145 joshLong145 merged commit bcb4a08 into master Jul 25, 2024
4 checks passed
@joshLong145 joshLong145 deleted the feature/lit-3536-cleanup-shiva-changes branch July 25, 2024 14:05
@joshLong145 joshLong145 mentioned this pull request Jul 26, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ansonhkg Ansonhkg Ansonhkg approved these changes

@jtary jtary Awaiting requested review from jtary

@FedericoAmura FedericoAmura Awaiting requested review from FedericoAmura

@MaximusHaximus MaximusHaximus Awaiting requested review from MaximusHaximus

Assignees
No one assigned
Labels
🐞 Bug Fix Something isn't working ✨ Enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@joshLong145 @Ansonhkg @MaximusHaximus