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

[09.2022] Bump Safe Dependencies #457

Merged
merged 6 commits into from Sep 23, 2022
Merged

[09.2022] Bump Safe Dependencies #457

merged 6 commits into from Sep 23, 2022

Conversation

bh2smith
Copy link
Owner

@bh2smith bh2smith commented Sep 1, 2022

As usual, safe apps can't be bumped by dependabot individually.

A few caveats with this update:

  1. We had to skip the version bump of @gnosis.pm/safe-react-components from v1.1.5 to v1.2.0 because of this change to their fonts and politely requested that they be put back.
  2. Observe that they recently removed two TransactionStatus enums (Fix: remove deprecated client-side tx statuses safe-global/safe-gateway-typescript-sdk#86) so we had to follow suit.
  3. There was an awkward built error in our hacky implementation of toWei where number.decimalPlaces() all of a sudden can return null. I wasn't able to tell when this happens, so we simply skip handling it.

@bh2smith
Copy link
Owner Author

bh2smith commented Sep 1, 2022

@schmanu - we didn't merge last month's (#448) Safe version bumps, so maybe we can just go with this one. Wouldn't want to fall behind on these! Since I created the PR, I can't approve it... would you like to take a look?

I am not super happy about the change to res.decimalPlaces() and would love if you had a better suggestion.

Copy link
Collaborator

@schmanu schmanu left a comment

Choose a reason for hiding this comment

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

For you open question on when res.decimalPlaces() can get 0:
It can be 0 if the BigNumber value is NaN or Infinity

So I think it's okay to kinda ignore it or handle it like an error as this should never happen.

We could also consider removing the toWei and fromWei functions because ethers has built in parseUnits and formatUnits functions which basically do the same.

Sorry for the late review. I forgot 🤦

@bh2smith
Copy link
Owner Author

bh2smith commented Sep 23, 2022

I have added a TODO in the code to replace toWEI and fromWEI with ethers.utils.PARSINGMETHOD, but that seems like a change we want to test well and doesn't belong here.

https://docs.ethers.io/v4/api-utils.html#ether-strings-and-wei

@bh2smith bh2smith merged commit d370956 into master Sep 23, 2022
@bh2smith bh2smith deleted the bump-safe-deps-2 branch September 23, 2022 12:46
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.

None yet

2 participants