Discussions #1050

Schwartz10 · 2019-06-26T13:42:52Z

No description provided.

…ite into discussions

* Clean up styles * Improve behavior

These buttons look good, but don't do anything yet

It needs a working save function in Discussion.js, but the rest is in place and it behaves properly on the frontend.

chadoh · 2019-08-22T14:10:55Z

apps/discussions/package.json

+    "publish:http": "npm run build:script && aragon apm publish major --http localhost:9999 --http-served-from ./dist",
+    "versions": "aragon apm versions",
+    "lint": "eslint . & solium --dir ./contracts",
+    "lint:fix": "eslint . --fix & solium --dir ./contracts --fix",


Should these two lines use && instead of &?

chadoh · 2019-08-22T14:11:15Z

apps/discussions/package.json

+  "description": "",
+  "dependencies": {
+    "@aragon/api": "latest",
+    "@aragon/api-react": "latest",


This differs from our other apps. Does that matter?

apps/discussions/app/App.js

Most of the linting errors were complaining about use of `now`. This [can cause security problems][1], if using `now` for security-related functions. We are not doing that here, so I am guessing that our use of `now` is safe, and am therefore ignoring these lint warnings explicitly. [1]: https://consensys.github.io/smart-contract-best-practices/recommendations/#timestamp-dependence

ottodevs · 2019-08-22T16:12:31Z

apps/discussions/contracts/DiscussionApp.sol

@@ -45,12 +45,12 @@ contract DiscussionApp is IForwarder, AragonApp {
        post.author = msg.sender;
        post.postCid = postCid;
        post.discussionThreadId = discussionThreadId;
-        post.createdAt = now;
+        post.createdAt = now; // solium-disable-line security/no-block-members


Just for future reference, we are using the TimeHelpers from AragonOS for these cases, so you can perfectly replace now for getTimestamp() or getTimestamp64() depending on what data type is post.createdAt (uint or uint64), but these kind of changes will likely will be applied on future audits to these contracts, so no worries! just to share the info :)

NOTE: The danger will be there anywhere, but it should be prevented at contract design stage, by avoiding using block members for critical outcomes, since these values are gameable by miners

Source: https://github.com/aragon/aragonOS/blob/next/contracts/common/TimeHelpers.sol#L36
(TimeHelpers members are automatically inherited by every AragonApp contract, and now is just an alias to block.timeStamp)

avoiding using block members for critical outcomes

Good to know about getTimestamp for AragonOS! In this case, I don't think the time at which a comment is created is a critical outcome, so I doubt it matters, but if I'd mistaken on that point I'd love for you to correct me!

Yeah I mean they're functionally identical, the main advantage is in testing since you can create a mock that overrides that call.

Fix lint and remove package-lock files

coveralls · 2019-08-22T18:12:02Z

Coverage remained the same at 100.0% when pulling c6212bc on discussions into 2ced247 on dev.

coveralls · 2019-08-22T18:12:02Z

Coverage remained the same at 100.0% when pulling a34c083 on discussions into 2ced247 on dev.

coveralls · 2019-08-22T18:12:02Z

Coverage remained the same at 100.0% when pulling a34c083 on discussions into 2ced247 on dev.

…kevin discussions Readme updates

This UI shows up when you select Discussions in the sidebar. We have nothing to show for the Discussions app itself at this point in time, and now we explain that here.

Quazia

A few small comments, I'm also not seeing any getters. This might not be an issue since all of the information needed is being emitted (which as a pattern I don't love) so this probably isn't an issue. Generally I think relying on events to pull all the contract information isn't a great pattern, but that's mainly for concurrency reasons where the data would be inaccurate if received out of sequence but I don't specifically see that being a problem here so I think it's okay.

You also don't have any tests for the contract that I'm seeing.

Quazia · 2019-08-23T04:59:53Z

apps/discussions/contracts/DiscussionApp.sol

+import "@aragon/os/contracts/common/IForwarder.sol";
+
+
+contract DiscussionApp is IForwarder, AragonApp {


Rename to Discussions to be conformant to contract naming conventions.

Quazia · 2019-08-23T05:03:39Z

apps/discussions/contracts/DiscussionApp.sol

@@ -45,12 +45,12 @@ contract DiscussionApp is IForwarder, AragonApp {
        post.author = msg.sender;
        post.postCid = postCid;
        post.discussionThreadId = discussionThreadId;
-        post.createdAt = now;
+        post.createdAt = now; // solium-disable-line security/no-block-members


Yeah I mean they're functionally identical, the main advantage is in testing since you can create a mock that overrides that call.

Quazia · 2019-08-23T05:04:45Z

apps/discussions/contracts/DiscussionApp.sol

+            discussionThreadId,
+            postId,
+            post.createdAt,
+            now // solium-disable-line security/no-block-members


This is redundant since the time information should be accessible from the event either way.

Quazia · 2019-08-23T05:07:02Z

apps/discussions/contracts/DiscussionApp.sol

+    event Hide(address indexed author, uint discussionThreadId, uint postId, uint hiddenAt);
+    event CreateDiscussionThread(uint actionId, bytes _evmScript);
+
+    bytes32 constant public DISCUSSION_POSTER_ROLE = keccak256("DISCUSSION_POSTER_ROLE");


Having a single role for all three actions isn't exactly ideal, what I would consider doing is splitting hide into two function calls with a seperate HIDE_DISCUSSION_ROLE and then keeping the one that exists but removing the role protections. As it is in most cases the auth check will have occured at the time of post, so this pattern only serves to prevent a poster from hiding their own post if they've had posting permissions removed. I think it makes more sense for hide to be something you can always do for your own posts and a separate role can do for other peoples posts.

Quazia · 2019-08-23T05:14:01Z

apps/discussions/contracts/DiscussionApp.sol

+
+    uint discussionThreadId;
+
+    mapping(uint => DiscussionPost[]) public discussionThreadPosts;


Not sure how this will behave on an upgrade... this may need to be a mapping into another mapping instead of an array.

Quazia · 2019-08-23T05:15:49Z

apps/discussions/contracts/DiscussionApp.sol

+        address author;
+        string postCid;
+        uint discussionThreadId;
+        uint id;


I would specify unit256 for all params, and for indexes or anything else that's only being modified based on incrementation you can probably get away with a uint64.

Quazia · 2019-08-23T05:28:12Z

apps/discussions/contracts/DiscussionApp.sol

+
+
+contract DiscussionApp is IForwarder, AragonApp {
+    using SafeMath for uint256;


Doesn't look like you're using this anywhere.

Quazia

Good for now, circle back to resolve the rest of the issues.

* dev: Dot voting demo fixes (#1176) Discussions (#1050) Fix linting and remove package-lock files

* newstyle: Add a minimal EMPTY_ROLE to Discussions - This is needed to be considered an actual Aragon app by the Kernel - Reference: https://hack.aragon.org/docs/cli-dao-commands#dao-install Remove partial ACL guards on discussions: Discussions: fix typos in README examples Dot voting demo fixes (#1176) Discussions (#1050) Fix linting and remove package-lock files

* newstyle: (47 commits) Revert Rewards & Projects to previous aragonUI Dot Voting: fix issues found during QA Dot Voting/CastVote: fix prop type Revert "[DEV] rm pre-commit lint" Dot Voting: mv totalSupport calc to VotingResults Dot Voting/VoteDetails: mv Participation Dot Voting/CastVote: rm tokenData; keep userBalance Dot Voting: define tokenContract in CastVote Dot Voting: mv CastVote to own file Dot Voting/VoteDetails: mv VoteEnact & VotingResults Add a minimal EMPTY_ROLE to Discussions - This is needed to be considered an actual Aragon app by the Kernel - Reference: https://hack.aragon.org/docs/cli-dao-commands#dao-install Remove partial ACL guards on discussions: Discussions: fix typos in README examples Dot voting demo fixes (#1176) Dot Voting/VoteDetails: mv DescriptionAndCreator Discussions (#1050) Dot Voting/VoteDetails: mv Title to own file Dot Voting/VoteDetail: mv Status to new file Dot Voting/VoteDetails: mv AppBadge to new file [DEV] rm pre-commit lint ...

* newstyle: (36 commits) Upgrade date fns (#1188) upgrade Rewards ADS UI (#1187) Aragon DS upgrade: addressbook (#1156) Revert Rewards & Projects to previous aragonUI Dot Voting: fix issues found during QA Dot Voting/CastVote: fix prop type Revert "[DEV] rm pre-commit lint" Dot Voting: mv totalSupport calc to VotingResults Dot Voting/VoteDetails: mv Participation Dot Voting/CastVote: rm tokenData; keep userBalance Dot Voting: define tokenContract in CastVote Dot Voting: mv CastVote to own file Dot Voting/VoteDetails: mv VoteEnact & VotingResults Add a minimal EMPTY_ROLE to Discussions - This is needed to be considered an actual Aragon app by the Kernel - Reference: https://hack.aragon.org/docs/cli-dao-commands#dao-install Remove partial ACL guards on discussions: Discussions: fix typos in README examples Dot voting demo fixes (#1176) Dot Voting/VoteDetails: mv DescriptionAndCreator Discussions (#1050) Dot Voting/VoteDetails: mv Title to own file ...

Schwartz10 and others added 30 commits June 18, 2019 11:00

paste in discussion app

f0bdba9

include discussions in npm start:dev build cmd

abe98f7

discussion app working

4640127

implement discussions as a useless forwarder

ff656f8

temp forwarding through discussions

7e0bf07

Merge branch 'scripts/no-client' of github.com:AutarkLabs/planning-su…

391f9b7

…ite into discussions

fix state malfunctions from new aragon-api

add3125

add sample external tx intent

beebf7b

start integration with new forwarder API

544ffd7

initial discussion state builder module framework

2d24958

build state for discussions in module

585709b

initial integration into dot voting

1d2f249

log discussion data

4ec7eb8

associate discussion thread with dot vote

1fa7198

display discussion panel and posts

151ff43

implement post functionality

3d3e962

sort discussion posts before serving to react

122bff0

small fixes

3ce089f

fix merge conflict

cae78ce

disable husky for time being

20ee8da

programmatically load discussions

a8c7ef3

update discussions as they occur

e7f2eb3

bug fix

99a75f0

Discussions: rm some UI that's not in design

1558049

Discussions: extract form to CommentForm

7ac68d0

* Clean up styles * Improve behavior

Discussions: DiscussionPost → Comment; styling

34fb477

Discussions/CommentForm: Add label

c144d02

Discussions/CommentForm: add hint text

c1a47d9

Discussions: add UI for edit & reply

f8a1523

These buttons look good, but don't do anything yet

Discussions: Hook up frontend of comment editing

e0fee02

It needs a working save function in Discussion.js, but the rest is in place and it behaves properly on the frontend.

chadoh removed request for a team, rkzel, ottodevs and topocount August 22, 2019 12:38

chadoh reviewed Aug 22, 2019

View reviewed changes

chadoh dismissed their stale review via fb66d35 August 22, 2019 14:41

chadoh reviewed Aug 22, 2019

View reviewed changes

apps/discussions/app/App.js Outdated Show resolved Hide resolved

chadoh force-pushed the discussions branch from fb66d35 to 4eafef5 Compare August 22, 2019 14:45

ottodevs added 2 commits August 22, 2019 17:49

Fix lint and remove package-lock files

b2a3986

Update lint script to use correct local config for each app

eb0613e

ottodevs reviewed Aug 22, 2019

View reviewed changes

Merge pull request #1179 from AutarkLabs/discussions-external-lint

a34c083

Fix lint and remove package-lock files

Schwartz10 and others added 4 commits August 22, 2019 14:47

Merge pull request #1172 from AutarkLabs/discussions-readme-comments-…

fef4306

…kevin discussions Readme updates

hardcode 100 discussion thread Ids

fbe3645

Discussions: add placeholder UI

19a104c

This UI shows up when you select Discussions in the sidebar. We have nothing to show for the Discussions app itself at this point in time, and now we explain that here.

Discussions: npm run lint:fix

39714ea

Quazia reviewed Aug 23, 2019

View reviewed changes

update contract

c6212bc

Quazia approved these changes Aug 23, 2019

View reviewed changes

Schwartz10 merged commit fc9f7fb into dev Aug 23, 2019

Schwartz10 deleted the discussions branch August 23, 2019 19:19

chadoh added a commit that referenced this pull request Aug 24, 2019

Merge branch 'dev' into newstyle

b1fa9b6

* dev: Dot voting demo fixes (#1176) Discussions (#1050) Fix linting and remove package-lock files

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Discussions #1050

Discussions #1050

Schwartz10 commented Jun 26, 2019

chadoh Aug 22, 2019

chadoh Aug 22, 2019

ottodevs Aug 22, 2019 •

edited

Loading

chadoh Aug 22, 2019

Quazia Aug 23, 2019

coveralls commented Aug 22, 2019 •

edited

Loading

coveralls commented Aug 22, 2019

coveralls commented Aug 22, 2019

Quazia left a comment

Quazia Aug 23, 2019

Quazia Aug 23, 2019

Quazia Aug 23, 2019

Quazia Aug 23, 2019

Quazia Aug 23, 2019

Quazia Aug 23, 2019

Quazia Aug 23, 2019

Quazia left a comment

		import "@aragon/os/contracts/common/IForwarder.sol";


		contract DiscussionApp is IForwarder, AragonApp {


		uint discussionThreadId;

		mapping(uint => DiscussionPost[]) public discussionThreadPosts;



		contract DiscussionApp is IForwarder, AragonApp {
		using SafeMath for uint256;

Discussions #1050

Discussions #1050

Conversation

Schwartz10 commented Jun 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ottodevs Aug 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 22, 2019 • edited Loading

coveralls commented Aug 22, 2019

coveralls commented Aug 22, 2019

Quazia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Quazia left a comment

Choose a reason for hiding this comment

ottodevs Aug 22, 2019 •

edited

Loading

coveralls commented Aug 22, 2019 •

edited

Loading