Skip to content

Allow 11 characters in symbol for RPC#10670

Merged
Gudahtt merged 3 commits intoMetaMask:developfrom
bakkerthehacker:longer-symbol-rpc
Mar 26, 2021
Merged

Allow 11 characters in symbol for RPC#10670
Gudahtt merged 3 commits intoMetaMask:developfrom
bakkerthehacker:longer-symbol-rpc

Conversation

@bakkerthehacker
Copy link
Contributor

@bakkerthehacker bakkerthehacker commented Mar 18, 2021

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
@bakkerthehacker bakkerthehacker requested a review from ryanml March 18, 2021 14:34
@github-actions
Copy link
Contributor

github-actions bot commented Mar 18, 2021 •
edited
Loading

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

bakkerthehacker commented Mar 18, 2021

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
Gudahtt
Gudahtt approved these changes Mar 24, 2021
View reviewed changes
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.

Reviewers

@Gudahtt Gudahtt Gudahtt approved these changes

@ryanml ryanml Awaiting requested review from ryanml

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

@bakkerthehacker @Gudahtt