trader-xyz / nft-swap-sdk

Ethereum's missing p2p NFT and token swap library for web3 developers. Written in TypeScript. Powered by 0x.
https://swapsdk.xyz
MIT License
219 stars 83 forks source link

ERC20 Allowance checks broken #97

Closed jeje closed 1 year ago

jeje commented 1 year ago

Hi,

I don't understand why the SDK expects almost an unlimited ERC20 allowance. For a buy offer, if the allowance is gte than the offered purchase price (+ optional fees), it should work.

I skipped the SDK functions for allowance checks but now I'm stuck with the orderbook which does the same checks.

How to reproduce:

johnrjj commented 1 year ago

Thanks, for reporting. We will add that in a future release -- definitely very valid feedback. I'll update the orderbook to only check for minimum required allowances.

jinsley8 commented 1 year ago

@johnrjj - Do you know if there is a workaround for this? @jeje Did you figure it out?

Metamask used to default to max spend but with a newer version update it now lets you specify spend by default.

Screenshot 2023-06-05 at 9 38 06 PM

If trading any ERC20 tokens then you have to click Use default to select max limit of 332306998946228968.225951765070086144

Otherwise if you put any custom value (larger than the ERC20 amount) or even if you click on Max which is the max amount in your wallet, the transaction will end up failing.

I've even tried allowance amounts of 1 WETH more than my wallet and it loadApprovalStatus() still detects it as unapproved.

jeje commented 1 year ago

@jeje Did you figure it out?

We are running our own orderbook instead as we needed other features like offchain auctions.

johnrjj commented 1 year ago

This is fixed with version 0.3.2 release!

jinsley8 commented 1 year ago

Sweet, thank you!

johnrjj commented 1 year ago

Linking release notes: https://github.com/trader-xyz/nft-swap-sdk/releases/tag/v0.32.0

johnrjj commented 1 year ago

Closed as fixed. Please open another issue if there's more feature requests!