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

Add numberRange() validator + string validation fixes #2220

Closed
taoeffect opened this issue Jul 18, 2024 · 2 comments · Fixed by #2274
Closed

Add numberRange() validator + string validation fixes #2220

taoeffect opened this issue Jul 18, 2024 · 2 comments · Fixed by #2274
Assignees
Labels
App:Frontend Note:Contracts Issues involving modifications to contracts Note:UI/UX Priority:High

Comments

@taoeffect
Copy link
Member

Problem

Some numbers should be appropriately limited in the app but aren't. Right now there is no simple way in contract validation functions to limit a number's range.

Also, I noticed separate missing stringMax validators for:

    'gi.contracts/group/proposalVote': {
      validate: actionRequireActiveMember(objectOf({
        proposalHash: stringMax(MAX_HASH_LEN, 'proposalHash'),
        vote: string,
    'gi.contracts/group/markProposalsExpired': {
      validate: actionRequireActiveMember(objectOf({
        proposalIds: arrayOf(string)
      })),
    'gi.contracts/group/removeMember': {
      validate: actionRequireActiveMember((data, { state, getters, message: { innerSigningContractID, proposalHash } }) => {
        objectOf({
          memberID: optional(string), // member to remove
          reason: optional(string),
          automated: optional(boolean)
        })(data)
        paymentMethods: arrayOf(
          objectOf({
            name: string,
            value: stringMax(GROUP_PAYMENT_METHOD_MAX_CHAR, 'paymentMethods.value')
          })
    'gi.contracts/group/leaveChatRoom': {
      validate: actionRequireActiveMember(objectOf({
        chatRoomID: stringMax(MAX_HASH_LEN, 'chatRoomID'),
        memberID: optional(string)
    'gi.contracts/group/joinChatRoom': {
      validate: actionRequireActiveMember(objectMaybeOf({
        memberID: optional(string),

Solution

Similar to stringMax(), implement numberRange() that looks like this:

numberRange(<low>, <high>)

No need to add the name of the parameter there. Just make sure that any error messages thrown include the range itself, and that should be sufficient to identity which parameter failed.

Then add this to:

  • mincomeAmount: make it go from 1 to 1000000000
  • distributionPeriodLength: from 1 to 365
  • amount (in group/payment)): from 0 to 1000000000
  • exchangeRate: from 0 to 1000000000
  • expires_date_ms: from 0 to Number.MAX_SAFE_INTEGER

For the stringMax validators:

  • vote should be a unionOf the 4 literalOfs from rules.js
  • proposalIds should be arrayOf MAX_HASH_LEN strings
  • memberID should be MAX_HASH_LEN
  • reason should be some value that's also enforced in the UI (double check that it is), and probably same as GROUP_DESCRIPTION_MAX_CHAR
  • paymentMethod name should be GROUP_NAME_MAX_CHAR
  • memberID is another MAX_HASH_LEN
@taoeffect taoeffect added App:Frontend Priority:High Note:Contracts Issues involving modifications to contracts labels Jul 18, 2024
@SebinSong
Copy link
Collaborator

@taoeffect
I implemented numberRange() in this previous PR.
So things left to do here is to add this to various places you mentioned.

Q. I remember you telling us a few weeks ago to refrain from making updates to contracts files and this one does involve changes in contract files. Is this safe to work on now?

@taoeffect
Copy link
Member Author

Yes, it is safe to work on the contract files. Please see the pinned comments on Slack for details about where caution should be exercised.

taoeffect pushed a commit that referenced this issue Aug 5, 2024
* add more validators

* add comment

* another comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
App:Frontend Note:Contracts Issues involving modifications to contracts Note:UI/UX Priority:High
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants