TruStory / truchain

⛓ A crypto-incentivized debate community
http://www.trustory.io
Other
33 stars 14 forks source link

Added the validation for the argument body max count #804

Open mohitmamoria opened 5 years ago

mohitmamoria commented 5 years ago

Works with https://github.com/TruStory/truapp/pull/84 in truapp.

jhernandezb commented 5 years ago

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 ?

shanev commented 5 years ago

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.

truted commented 5 years ago

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:

  1. Update this PR and remove strip markdown, but check that argument length <= ArgumentBodyMaxLength
  2. 5000 characters argument body limit on chain (can update using CLI)
  3. 1500 stripped markdown character limit on client
codecov[bot] commented 5 years ago

Codecov Report

Merging #804 into develop will decrease coverage by 0.02%. The diff coverage is 0%.

@@             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
mohitmamoria commented 5 years ago

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.