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

feat(transfer to L2): max amount #62

Merged
merged 12 commits into from
Jan 18, 2022
Merged

feat(transfer to L2): max amount #62

merged 12 commits into from
Jan 18, 2022

Conversation

dan-ziv
Copy link
Collaborator

@dan-ziv dan-ziv commented Jan 12, 2022

Description of the Changes

Please add a detailed description of the change, whether it's an enhancement or a bugfix.
If the PR is related to an open issue please link to it.

Checklist

  • Changes have been done against master branch, and PR does not conflict
  • New unit / functional tests have been added (whenever applicable)
  • Test are passing in local environment
  • Docs have been updated
  • PR title is follow the standard-version convention: <type>(optional subject): <description>, e.g: fix: minor typos in code

This change is Reviewable

@dan-ziv dan-ziv requested a review from dan-starkware January 12, 2022 19:54
@dan-ziv dan-ziv self-assigned this Jan 12, 2022
@dan-ziv dan-ziv changed the title feat: max amount feat(transfer to L2): max amount Jan 12, 2022
Base automatically changed from chore/change-terminology to dev January 12, 2022 19:56
Copy link
Collaborator

@dan-starkware dan-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dan-ziv)


src/hooks/useTransfer.js, line 74 at r1 (raw file):

          handleError(
            progressOptions.error(
              new Error(`You are not allow to transfer more then ${maxAmount} ${symbol}`)

Consider adding a . at the end.

Also, consider not letting the user choose a higher amount, to begin with. (maybe a slider with bounds [1, maxAmount])

Code quote:

You are not allow to transfer more then ${maxAmount} ${symbol}

@dan-ziv
Copy link
Collaborator Author

dan-ziv commented Jan 17, 2022


src/hooks/useTransfer.js, line 74 at r1 (raw file):

Previously, dan-starkware wrote…

Consider adding a . at the end.

Also, consider not letting the user choose a higher amount, to begin with. (maybe a slider with bounds [1, maxAmount])

Slider is no good with big numbers. For this we have the input field.
I less like it as it async operation, and it will not let the user to enter amount until it will be finished.
@bbrandtom in which way do you want to validate it?

@dan-ziv dan-ziv requested a review from dan-starkware January 17, 2022 11:19
@dan-ziv dan-ziv linked an issue Jan 17, 2022 that may be closed by this pull request
Copy link
Member

@bbrandtom bbrandtom left a comment

Choose a reason for hiding this comment

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

Text LGTM - I didn't review the code

Copy link
Collaborator

@dan-starkware dan-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 7 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @bbrandtom)

@dan-ziv dan-ziv merged commit aeefa55 into dev Jan 18, 2022
@dan-ziv dan-ziv deleted the feat/max-amount branch January 18, 2022 07:46
@dan-ziv dan-ziv restored the feat/max-amount branch September 6, 2022 11:18
@dan-ziv dan-ziv deleted the feat/max-amount branch September 6, 2022 11:20
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.

feat: validate maximum token amount to transfer
3 participants