-
Notifications
You must be signed in to change notification settings - Fork 14
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
Added the validation for the argument body max count #804
base: develop
Are you sure you want to change the base?
Conversation
Not really sure if we want the chain to be content specific aware , in this case markdown. I think the solution previously discussed was increasing the limit in the chain but keep it in the client (truapi/truapp) and the limit could be checked in truapi (strip markdown, etc), not really sure how well this would work thoughts @shanev ? |
Maybe in the future the chain can accept different content types, but for now importing a markdown library limits the generality of the blockchain. Other clients should be free to use HTML or whatever text based content type they want. So I say we go with the solution we talked about previously of making the length fixed on the chain, and adding smarts to the client to not go over this limit. Lowers attack vectors as well. |
I propose:
|
Codecov Report
@@ Coverage Diff @@
## develop #804 +/- ##
===========================================
- Coverage 58.78% 58.76% -0.03%
===========================================
Files 73 73
Lines 4164 4166 +2
===========================================
Hits 2448 2448
- Misses 1507 1508 +1
- Partials 209 210 +1 |
Okay, so I have updated this PR to check the body length. Would request @shanev to update the params on the beta and devnet (so that I don't mess something up). Have overridden this limit on the client in https://github.com/TruStory/truapp/pull/84. |
Works with https://github.com/TruStory/truapp/pull/84 in
truapp
.