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

[Gnosis]: Various bug fixes #624

Merged
merged 7 commits into from
Aug 4, 2021
Merged

[Gnosis]: Various bug fixes #624

merged 7 commits into from
Aug 4, 2021

Conversation

evgenyboxer
Copy link
Contributor

@evgenyboxer evgenyboxer commented Aug 4, 2021 •
edited
Loading

Description

Various bug fixes from testing:

  1. FiatCurrency enum typo.
  2. Clear state after a trade.
  3. Pass token address to balanceFor method
  4. Update SOR when liquidity source changes
  5. Disable loading indicator when the trade has a quote (it still fetches it in the background on every block)

Refactor:

  1. Add a new "wrapUnwrap" to simplify logic (instead of doing "isBalancerTrade && !isWrapOrUnwrap"

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)
  • Other

How should this be tested?

Please provide instructions so we can test. Please also list any relevant details for your test configuration.

  • Mostly ERC20<>ERC20 trades.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code where relevant, particularly in hard-to-understand areas
  • My changes generate no new console warnings
  • The base of this PR is master if hotfix, develop if not

@vercel
Copy link

vercel bot commented Aug 4, 2021 •
edited
Loading

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

kovan-app – ./

🔍 Inspect: https://vercel.com/balancer/kovan-app/Agsh3GQAG8cHjT18EJwE4tkSGWB1
✅ Preview: Canceled

[Deployment for f527baf canceled]

app – ./

🔍 Inspect: https://vercel.com/balancer/app/DtpypFh1gg2YeCcLhu2VsxTNrAW6
✅ Preview: Canceled

[Deployment for f527baf canceled]

staging-app – ./

🔍 Inspect: https://vercel.com/balancer/staging-app/BxsZa7oTWBAUt9vPbtqVou6B1auk
✅ Preview: https://staging-app-git-fix-gnosis-bugs-various-balancer.vercel.app

staging-kovan-app – ./

🔍 Inspect: https://vercel.com/balancer/staging-kovan-app/Gwk3wq6SXsooMxyrCnVmuiiQWF3q
✅ Preview: https://staging-kovan-app-git-fix-gnosis-bugs-various-balancer.vercel.app

gnosis – ./

🔍 Inspect: https://vercel.com/balancer/gnosis/HG1oaQGifcGFE1FwrGtwUenpyL38
✅ Preview: https://gnosis-git-fix-gnosis-bugs-various-balancer.vercel.app

polygon – ./

🔍 Inspect: https://vercel.com/balancer/polygon/2oWWHCuBAWxV5PTPwiGKSyNBSLL8
✅ Preview: https://polygon-git-fix-gnosis-bugs-various-balancer.vercel.app

beta-polygon – ./

🔍 Inspect: https://vercel.com/balancer/beta-polygon/8v6L2Dp2zKazehoV2DhpYQLCTWkx
✅ Preview: https://beta-polygon-git-fix-gnosis-bugs-various-balancer.vercel.app

@vercel vercel bot temporarily deployed to Preview – app August 4, 2021 01:30 Inactive
@vercel vercel bot temporarily deployed to Preview – kovan-app August 4, 2021 01:30 Inactive
evgenyboxer
evgenyboxer commented Aug 4, 2021
View reviewed changes
src/composables/trade/useGnosis.ts
@@ -296,7 +296,7 @@ export default function useGnosis({

state.errors.priceExceedsBalance = bnum(
formatUnits(maximumInAmount, tokenIn.value.decimals)
).gt(balanceFor(tokenIn.value));
).gt(balanceFor(tokenIn.value.address));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@garethfuller I've added a type for balanceFor - I think we should type all the other methods too, so we can count on TS to catch those :)

@evgenyboxer evgenyboxer force-pushed the fix/gnosis-bugs-various branch from 702664a to 9c74794 Compare August 4, 2021 01:34
@vercel vercel bot temporarily deployed to Preview – kovan-app August 4, 2021 01:34 Inactive
@vercel vercel bot temporarily deployed to Preview – app August 4, 2021 01:34 Inactive
@vercel vercel bot temporarily deployed to Preview – kovan-app August 4, 2021 01:37 Inactive
@vercel vercel bot temporarily deployed to Preview – app August 4, 2021 01:37 Inactive
@vercel vercel bot temporarily deployed to Preview – app August 4, 2021 03:13 Inactive
@vercel vercel bot temporarily deployed to Preview – kovan-app August 4, 2021 03:13 Inactive
@vercel vercel bot temporarily deployed to Preview – app August 4, 2021 03:18 Inactive
@vercel vercel bot temporarily deployed to Preview – kovan-app August 4, 2021 03:20 Inactive
@vercel vercel bot temporarily deployed to Preview – kovan-app August 4, 2021 05:02 Inactive
@vercel vercel bot temporarily deployed to Preview – app August 4, 2021 05:02 Inactive
@evgenyboxer evgenyboxer merged commit 48dfc3f into develop Aug 4, 2021
@evgenyboxer evgenyboxer deleted the fix/gnosis-bugs-various branch August 4, 2021 05:21
TomAFrench added a commit that referenced this pull request Aug 7, 2021
@TomAFrench
Merge branch 'develop' into safe-app
04df591 
* develop:
  [Gnosis]: Add wrap/unwrap transaction action (#636)
  fix: use more accurate buy/sell amounts for transactions (#635)
  Add support for transaction speed ups / cancellations (#631)
  UI 678 lm table (#621)
  Cleanup dependencies (#629)
  Disable coingecko list for now (#628)
  Cleanup unused files & functions (#626)
  Hotfix/1.11.4 (#627)
  [Gnosis]: Various bug fixes (#624)
  Registry refactor hook up (#570)
  Persist transactions (#620)
  [Gnosis]: More UI Tweaks (#616)
This was referenced Aug 24, 2021
Merged
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arb000r arb000r arb000r approved these changes

@garethfuller garethfuller Awaiting requested review from garethfuller

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@evgenyboxer @arb000r