kredits/contracts
kredits/contracts
9
1
Fork 0
Code Issues 21 Pull Requests Actions Projects 1 Releases 8 Activity

Refactor contribution serializer and validation #94

Merged
raucao merged 4 commits from bugfix/schema_changes into master 2019-04-12 18:54:57 +00:00
Conversation 10 Commits 4 Files Changed 8 +112 -77
raucao commented 2019-04-11 11:06:42 +00:00 (Migrated from github.com)
Copy Link

And fix seeds.

And fix seeds.
raucao commented 2019-04-11 11:08:23 +00:00 (Migrated from github.com)
Copy Link

I set this to WIP, because I'm going to add proposal validation and valid proposal seeds to this as well.

I set this to WIP, because I'm going to add proposal validation and valid proposal seeds to this as well.
raucao commented 2019-04-11 11:51:35 +00:00 (Migrated from github.com)
Copy Link

Ready for review!

Ready for review!
fsmanuel (Migrated from github.com) reviewed 2019-04-11 11:58:35 +00:00
lib/serializers/contribution.js
@ -72,1 +51,4 @@
}
/**
* Validate serialized data against schema
fsmanuel (Migrated from github.com) commented 2019-04-11 11:58:35 +00:00
Copy Link

What about:

  serialize (stringify=true) {
    // the current implementation

    return stringify ? JSON.stringify(data, null, 2) : data;
  }

  validate () {
    const serialized = this.serialize(false);
    const valid = validator.validate(serialized, schemas['contribution']);
    return valid ? Promise.resolve() : Promise.reject(validator.error);
  }
What about: ```js serialize (stringify=true) { // the current implementation return stringify ? JSON.stringify(data, null, 2) : data; } validate () { const serialized = this.serialize(false); const valid = validator.validate(serialized, schemas['contribution']); return valid ? Promise.resolve() : Promise.reject(validator.error); } ```
bumi (Migrated from github.com) approved these changes 2019-04-11 21:34:35 +00:00
bumi (Migrated from github.com) left a comment
Copy Link

:shipit:
@fsmanuel what's with your comment, want to merge it?

:shipit: @fsmanuel what's with your comment, want to merge it?
raucao (Migrated from github.com) reviewed 2019-04-12 07:07:34 +00:00
lib/serializers/contribution.js
@ -72,1 +51,4 @@
}
/**
* Validate serialized data against schema
raucao (Migrated from github.com) commented 2019-04-12 07:07:34 +00:00
Copy Link

Let's try ro create smaller PRs for suggestions like this, instead of having them hold up larger branches from being merged.

(E.g. personally, I don't like unnamed options, so I'd probably propose to use an options object for this. But that's a completely different discussion than if the schema validation here is implemented good enough in general or not.)

What do you think?

Let's try ro create smaller PRs for suggestions like this, instead of having them hold up larger branches from being merged. (E.g. personally, I don't like unnamed options, so I'd probably propose to use an options object for this. But that's a completely different discussion than if the schema validation here is implemented good enough in general or not.) What do you think?
fsmanuel (Migrated from github.com) reviewed 2019-04-12 11:27:10 +00:00
lib/serializers/contribution.js
@ -72,1 +51,4 @@
}
/**
* Validate serialized data against schema
fsmanuel (Migrated from github.com) commented 2019-04-12 11:27:10 +00:00
Copy Link

Well I think that the initial idea to move the validation into the serializer was to get rid of the JSON.stringify ->JSON.parse thing for the validation. My suggestion is focusing on how we can prevent this.
Another thing I don't understand is why you removed the static function and don't use the class at all.

Well I think that the initial idea to move the validation into the serializer was to get rid of the `JSON.stringify` ->`JSON.parse` thing for the validation. My suggestion is focusing on how we can prevent this. Another thing I don't understand is why you removed the static function and don't use the class at all.
raucao (Migrated from github.com) reviewed 2019-04-12 11:32:57 +00:00
lib/serializers/contribution.js
@ -72,1 +51,4 @@
}
/**
* Validate serialized data against schema
raucao (Migrated from github.com) commented 2019-04-12 11:32:57 +00:00
Copy Link

Well I think that the initial idea to move the validation into the serializer was to get rid of the JSON.stringify ->JSON.parse thing for the validation. My suggestion is focusing on how we can prevent this.

That's not at all how I understood the idea. I merely refactored everything to be both shared modules and also do the validation in the class instead of outside.

Another thing I don't understand is why you removed the static function and don't use the class at all.

Not sure I understand. It's using only the class now, because I added more functions to it. I turned the static function into a prototype function, because that's actually using the class with prototype properties then.

> Well I think that the initial idea to move the validation into the serializer was to get rid of the `JSON.stringify` ->`JSON.parse` thing for the validation. My suggestion is focusing on how we can prevent this. That's not at all how I understood the idea. I merely refactored everything to be both shared modules and also do the validation in the class instead of outside. > Another thing I don't understand is why you removed the static function and don't use the class at all. Not sure I understand. It's using only the class now, because I added more functions to it. I turned the static function into a prototype function, because that's actually using the class with prototype properties then.
raucao (Migrated from github.com) reviewed 2019-04-12 11:33:53 +00:00
lib/serializers/contribution.js
@ -72,1 +51,4 @@
}
/**
* Validate serialized data against schema
raucao (Migrated from github.com) commented 2019-04-12 11:33:53 +00:00
Copy Link

By the way, in a way I also like validating exactly what's being stored in IPFS. I don't see it as a problem, and I don't see why a refactoring of that should keep us from merging now, as explained. I'm open to good reasons why the code in this PR doesn't work or is so bad it needs to be changed. But frankly, it seems like wasting time to me. This refactoring wouldn't even affect the code using the class from the outside. Let alone clients using the wrapper.

By the way, in a way I also like validating *exactly* what's being stored in IPFS. I don't see it as a problem, and I don't see why a refactoring of that should keep us from merging now, as explained. I'm open to good reasons why the code in this PR doesn't work or is so bad it needs to be changed. But frankly, it seems like wasting time to me. This refactoring wouldn't even affect the code using the class from the outside. Let alone clients using the wrapper.
fsmanuel (Migrated from github.com) reviewed 2019-04-12 12:33:24 +00:00
lib/serializers/contribution.js
@ -72,1 +51,4 @@
}
/**
* Validate serialized data against schema
fsmanuel (Migrated from github.com) commented 2019-04-12 12:33:24 +00:00
Copy Link

I was referring to this https://github.com/67P/kredits-contracts/pull/92#discussion_r274168386
So my suggestion adds little to archive this. I modified to code above to better illustrate the idea. If you disagree merge it as is.

Let's try to create smaller PRs for suggestions like this, instead of having them hold up larger branches from being merged.

I think reviews are the place to raise this discussions, aren't they?

I was referring to this https://github.com/67P/kredits-contracts/pull/92#discussion_r274168386 So my suggestion adds little to archive this. I modified to code above to better illustrate the idea. If you disagree merge it as is. > Let's try to create smaller PRs for suggestions like this, instead of having them hold up larger branches from being merged. I think reviews are the place to raise this discussions, aren't they?
fsmanuel (Migrated from github.com) reviewed 2019-04-12 12:35:00 +00:00
lib/serializers/contribution.js
@ -72,1 +51,4 @@
}
/**
* Validate serialized data against schema
fsmanuel (Migrated from github.com) commented 2019-04-12 12:35:00 +00:00
Copy Link

That being said. I like your implementation!

That being said. I like your implementation!
Sign in to join this conversation.
Reviewers
No Reviewers
bumi
Labels
Clear labels
good first issue
Good for newcomers
ipfs
rsk
scaling
bug
Something is not working
dev environment
Config, builds, CI, deployment, etc.
docs
Documentation
duplicate
This issue or pull request already exists
enhancement
Improving existing functionality
feature
New functionality
idea
Something to consider
invalid
Not a bug
kredits-1
Small contribution
kredits-2
Medium contribution
kredits-3
Large contribution
question
Looking for an answer
release
major
release
minor
release
patch
security
All your base are belong to us
ui/ux
User interface, process design, etc.
wontfix
This won't be fixed
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
bkero bumi fsmanuel galfert greg haythem hueso raucao slvrbckt
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: kredits/contracts#94
No description provided.
Delete Branch "bugfix/schema_changes"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?