Skip to content

Conversation

@mahmud-bn
Copy link
Contributor

@mahmud-bn mahmud-bn commented Jul 28, 2022

Description

This PR:

  • enforces the weiToETH function to accept only strings from core and common
  • returns ethers.BigNumber for GetInterfaceHelpers used by Ledger

Checklist

  • The version field in package.json is incremented following semantic versioning
  • The box that allows repo maintainers to update this PR is checked
  • I tested locally to make sure this feature/fix works
  • I have run yarn type-check & yarn build to confirm there are not any associated errors
  • This PR passes the Circle CI checks

Joi.number().positive()).required,
id: Joi.string()
.pattern(/^0x[0-9a-fA-F]+$/)
.required(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to change this validation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was related to the validation error fix for hardware wallets chain added to the main release:

There is another Joi.alternatives- string or number validation included in the previous PR core chainID which .

I guess placing chains: Chain[] | ChainWithDecimalId[] will allow for such validation but I'm not sure that's needed since the GetInterfaceHelpers used by Ledger works with the internal hex string.

Copy link
Contributor

@Adamj1232 Adamj1232 Jul 29, 2022

Choose a reason for hiding this comment

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

This seem correct if we are using Hex chain IDs internally(converting number chain ids to hex after initialization) it doesn't seem we would need positive number validation within the common package although I may be misunderstanding the possible flows of potential chainId

@mahmud-bn mahmud-bn requested a review from lnbc1QWFyb24 July 29, 2022 04:16
@Adamj1232 Adamj1232 self-requested a review July 29, 2022 18:51
@lnbc1QWFyb24
Copy link
Contributor

Do we also need to remove the bignumber.js dep from core and the rollup externals since it is not being used anymore?

@mahmud-bn
Copy link
Contributor Author

Do we also need to remove the bignumber.js dep from core and the rollup externals since it is not being used anymore?

It's still used by preflight-notifications.ts and notify.ts

@Adamj1232 Adamj1232 merged commit 9e01c83 into v2-web3-onboard-develop Aug 1, 2022
@Adamj1232 Adamj1232 deleted the fix/weitoEth-string branch August 1, 2022 20:57
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