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

Added the validation for the argument body max count #804

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

mohitmamoria
Copy link
Contributor

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

@jhernandezb
Copy link
Contributor

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
Copy link
Contributor

shanev commented Oct 3, 2019

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
Copy link
Contributor

truted commented Oct 3, 2019

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
Copy link

codecov bot commented Oct 4, 2019

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
Copy link
Contributor Author

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.

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.

4 participants