okTurtles / group-income

A decentralized and private (end-to-end encrypted) financial safety net for you and your friends.
https://groupincome.org
GNU Affero General Public License v3.0
331 stars 44 forks source link

Add `numberRange()` validator + string validation fixes #2220

Closed taoeffect closed 3 months ago

taoeffect commented 4 months ago

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:

For the stringMax validators:

SebinSong commented 4 months ago

@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 commented 4 months ago

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.