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

Allow 11 characters in symbol for RPC #10670

Merged
merged 3 commits into from
Mar 26, 2021

Conversation

bakkerthehacker
Copy link
Contributor

To match the symbol length requirements in the UI, the RPC symbol length checks have been updated to allow 11 character symbols and disallow the empty string as a symbol.

@bakkerthehacker bakkerthehacker requested a review from a team as a code owner March 18, 2021 14:34
@github-actions
Copy link
Contributor

github-actions bot commented Mar 18, 2021

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.

@bakkerthehacker
Copy link
Contributor Author

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

@Gudahtt
Copy link
Member

Gudahtt commented Mar 24, 2021

Thanks for the contribution! This is something that we want to do, but we need to ensure this change is made roughly at the same time for metamask-mobile as well, to ensure that eth_watchAsset doesn't break on mobile if dapps update to take advantage of longer symbols. I'll be coordinating with the mobile team to support that effort.

@Gudahtt Gudahtt added the DO-NOT-MERGE Pull requests that should not be merged label Mar 24, 2021
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

I've labelled this as "DO_NOT_MERGE" until we coordinate with mobile on making this change there as well, but these changes all look good to me 👍

@Gudahtt Gudahtt added blocked and removed DO-NOT-MERGE Pull requests that should not be merged blocked labels Mar 24, 2021
@Gudahtt
Copy link
Member

Gudahtt commented Mar 26, 2021

The mobile implementation of this has been implemented here: MetaMask/core#433
So this is now ready to merge.

@Gudahtt Gudahtt merged commit 94fc420 into MetaMask:develop Mar 26, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Mar 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants