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: Make the valid Token Symbol Length Parametric #17642

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

miladtsx
Copy link

@miladtsx miladtsx commented Feb 7, 2023

This pull request is to resolve issue #9243.

@miladtsx miladtsx requested a review from a team as a code owner February 7, 2023 15:54
@miladtsx miladtsx requested a review from jpuri February 7, 2023 15:54
@github-actions
Copy link
Contributor

github-actions bot commented Feb 7, 2023

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@miladtsx
Copy link
Author

miladtsx commented Feb 7, 2023

I have read the CLA Document and I hereby sign the CLA

@miladtsx miladtsx changed the title Remove the Token Symbol length limit. #9243 Increase Token Symbol Length Feb 7, 2023
@miladtsx miladtsx force-pushed the remove_symbol_length_limit_9243 branch from 83278d1 to 2456926 Compare February 7, 2023 19:28
@miladtsx miladtsx changed the title Increase Token Symbol Length Increase Token Symbol Length to 100 Feb 8, 2023
@miladtsx miladtsx changed the title Increase Token Symbol Length to 100 Increase Token Symbol Length to 100 Characters Feb 8, 2023
@miladtsx miladtsx force-pushed the remove_symbol_length_limit_9243 branch from 2456926 to dc2de82 Compare February 22, 2023 09:18
@matrixise
Copy link

ping

@legobeat legobeat force-pushed the remove_symbol_length_limit_9243 branch from dc2de82 to 162fc2d Compare April 27, 2023 04:07
@@ -367,8 +367,8 @@ class ImportToken extends Component {
const symbolLength = customSymbol.length;
let customSymbolError = null;

if (symbolLength <= 0 || symbolLength >= 12) {
customSymbolError = this.context.t('symbolBetweenZeroTwelve');
if (symbolLength === 0 || symbolLength >= 100) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If this could be replaced with a configuration variable, this could be changed in teh future without requiring any changes to localization files.

See how other messages utilize templating by $1,$2, etc to create dynamic templates, where the max-length could be a new template value here.

Copy link
Author

Choose a reason for hiding this comment

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

@legobeat good idea! it is parametric now.

@miladtsx miladtsx force-pushed the remove_symbol_length_limit_9243 branch from 97d28c9 to 57acbca Compare May 8, 2023 07:05
@miladtsx miladtsx changed the title Increase Token Symbol Length to 100 Characters Make the valid Token Symbol Length Parametric May 8, 2023
@miladtsx miladtsx requested a review from legobeat May 8, 2023 07:35
@github-actions
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity in the last 60 days. It will be closed in 14 days. Thank you for your contributions.

@github-actions github-actions bot added stale issues and PRs marked as stale and removed stale issues and PRs marked as stale labels Jul 20, 2023
@github-actions
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity in the last 60 days. It will be closed in 14 days. Thank you for your contributions.

@github-actions github-actions bot added the stale issues and PRs marked as stale label Sep 23, 2023
@legobeat legobeat requested review from a team September 24, 2023 23:41
@github-actions github-actions bot removed the stale issues and PRs marked as stale label Sep 25, 2023
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity in the last 60 days. It will be closed in 14 days. Thank you for your contributions.

@fredwes
Copy link

fredwes commented May 27, 2024

Sorry to keep bumping here but our users have reported issues adding tokens due to this. @legobeat any update on getting this merged in?

@legobeat
Copy link
Contributor

Sorry to keep bumping here but our users have reported issues adding tokens due to this. @legobeat any update on getting this merged in?

@fredwes Unfortunately, I don't have any further insights on the progress to share at this time.

@MetaMask/extension-devs Could we get some eyes on this, please? Any UI/UX concerns?

@vthomas13 vthomas13 added the needs-assets-ux-review A shared label between the Assets and UX team to flag PRs ready for consolidated team review. label May 28, 2024
@bergeron
Copy link
Contributor

bergeron commented Jun 3, 2024

This is a good idea, and MetaMask should support longer symbol lengths. There are other parts of the codebase that would need to be updated. At least here but maybe others. And it would require some UI work to consider every place showing symbols to make sure they render correctly, and do something like an ... ellipses or truncation to make it fit. So I don't think this can be merged as is.

Copy link
Contributor

github-actions bot commented Aug 2, 2024

This PR has been automatically marked as stale because it has not had recent activity in the last 60 days. It will be closed in 14 days. Thank you for your contributions.

@github-actions github-actions bot added stale issues and PRs marked as stale and removed stale issues and PRs marked as stale labels Aug 2, 2024
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

The pull request updates localization files to support a parametric token symbol length, increasing the limit to 20 characters.

  • Updated validation message for token symbol length in app/_locales/et/messages.json to be parametric.
  • Similar updates made across multiple localization files, including app/_locales/cs/messages.json, app/_locales/am/messages.json, and others.
  • Removed old fixed-length messages and replaced them with dynamic messages using placeholders.
  • Ensured consistency in validation logic across all affected localization files.
  • No security issues or logical errors identified; changes are straightforward localization updates.

30 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

Copy link

codecov bot commented Aug 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.10%. Comparing base (91dc6ea) to head (8d813c1).

Additional details and impacted files
@@           Coverage Diff            @@
##           develop   #17642   +/-   ##
========================================
  Coverage    70.10%   70.10%           
========================================
  Files         1430     1430           
  Lines        50147    50148    +1     
  Branches     13872    13872           
========================================
+ Hits         35152    35153    +1     
  Misses       14995    14995           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

This pull request implements changes to increase the token symbol length limit from 12 to 20 characters, addressing issue #9243. The main changes include:

  • Updated localization files to support a parametric token symbol length, replacing fixed-length messages with dynamic ones using placeholders.
  • Modified the ConfirmInfoRowTextTokenUnits component to handle longer token amounts by implementing a shortenString function that truncates long amounts to 15 characters with a tooltip showing the full amount.
  • Removed auto-detect token and NFT modals, suggesting a shift in how token detection is handled.
  • Updated various test files to accommodate the new token symbol length limit.
  • Made changes to Snap-related components and utilities, which may indirectly support the new token symbol length.

Key points to note:

  • The changes are primarily focused on UI components and localization, ensuring consistent handling of longer token symbols across the application.
  • The implementation follows the approach suggested in the issue, making the token symbol length parametric rather than hard-coded.
  • While many files were touched, the core changes related to token symbol length are concentrated in a few key areas, particularly localization and the ConfirmInfoRowTextTokenUnits component.
  • The removal of auto-detect token functionality may be related to accommodating longer token symbols, but this change might require further explanation or documentation.

Overall, the changes appear to successfully implement the requested feature while maintaining consistency across the application. However, it would be beneficial to have more context on the removal of auto-detect functionality and its relation to the increased token symbol length.

288 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

@miladtsx miladtsx requested review from a team as code owners August 21, 2024 21:06
@miladtsx
Copy link
Author

Hey @bergeron, thanks for your feedback.
I made some UI adjustments and tested them manually as much as possible.

I also adjusted MetaMask Core to support this change.

Looking forward to your feedback.

Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity in the last 60 days. It will be closed in 14 days. Thank you for your contributions.

@github-actions github-actions bot added the stale issues and PRs marked as stale label Nov 11, 2024
@legobeat
Copy link
Contributor

not stale

@github-actions github-actions bot removed the stale issues and PRs marked as stale label Nov 13, 2024
@aspiers
Copy link

aspiers commented Nov 13, 2024

Correct, not stale, and this stale bot is also counter-productive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external-contributor INVALID-PR-TEMPLATE PR's body doesn't match template needs-assets-ux-review A shared label between the Assets and UX team to flag PRs ready for consolidated team review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants