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(core-p2p): increase maximum token symbol length #3311

Merged
merged 2 commits into from
Dec 5, 2019
Merged

feat(core-p2p): increase maximum token symbol length #3311

merged 2 commits into from
Dec 5, 2019

Conversation

Nigui
Copy link
Contributor

@Nigui Nigui commented Nov 29, 2019

Summary

In my bridgechain I have a token symbol with length 4 and current p2p communication schema breaks peers exchanges.

Reasons why I have a 4 characters symbol is :

  • I don't have unicode representing my token (like Ѧ does)
  • It's on a test network (devnet, testnet, unitnet,...) and I've just prefixed my token trigram with first letter of network (similarily to DѦ, TѦ and UѦ).

I don't think I'm and I'll be the only bridgechain facing this issue.

My solution is to increment symbol length by 1 only to fit with my current needs.

Maybe a better solution would be to remove that check ?

Checklist

  • Documentation (if necessary)
  • Tests (if necessary)
  • Ready to be merged

@ghost
Copy link

ghost commented Nov 29, 2019

Thanks for submitting this pull request! A maintainer will review this in the next few days and explicitly select labels so you know what's going on.

If no reviewer appears after a week, a reminder will be sent out.

@ghost ghost added Complexity: Low labels Nov 29, 2019
@fix
Copy link
Contributor

fix commented Dec 4, 2019

good point!, i think we need to have a db migration for that, but nothing unachievable

@faustbrian
Copy link
Contributor

@Nigui we can increase this to something reasonable like a maximum of 8 characters to allow some longer strings but removing this check completely would mean that you could send any arbitrary data of any length in that specific field.

Don't understand how we need a database migration for a validation schema so just ignore that comment from @fix.

@fix
Copy link
Contributor

fix commented Dec 4, 2019

oh yes, i thought there were some database table with the data. so yes no database schema migration then :)

Copy link
Contributor

@faustbrian faustbrian left a comment

Choose a reason for hiding this comment

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

Will merge it with 4 for now as some exchanges have a limit on token length. Will increase it further if necessary.

@ghost
Copy link

ghost commented Dec 5, 2019

A collaborator has approved this PR. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait.

Thank you for your contribution!

@faustbrian faustbrian merged commit e55602d into ArkEcosystem:develop Dec 5, 2019
@ghost
Copy link

ghost commented Dec 5, 2019

Your pull request has been merged. Thank you for your contribution to the ARK Ecosystem.

@Nigui Nigui deleted the patch-1 branch December 5, 2019 07:57
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.

3 participants