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

Contract updates to match new frontends #28

Merged
merged 44 commits into from
Jun 7, 2023
Merged

Contract updates to match new frontends #28

merged 44 commits into from
Jun 7, 2023

Conversation

T-Damer
Copy link
Contributor

@T-Damer T-Damer commented May 25, 2023

NOTE: Reactions and comments point to postId, and we mixed up threadId and postId on frontend, so we decided to use postId name here

@T-Damer T-Damer self-assigned this May 25, 2023
@@ -83,11 +83,8 @@ contract KetlGuarded is Initializable, OwnableUpgradeable {
allowedCaller = _allowedCaller;
}

// @Todo: rollback when release
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just pointing out

@T-Damer T-Damer changed the title Don't mix postId and threadId, use only postId Contract updates to match new frontends May 29, 2023
parentPost.numberOfComments++;
posts[feedId][postId].numberOfComments++;
if (replyTo > 0) {
comments[feedId][postId][replyTo - 1].numberOfComments++;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to update the comment's number of comments? I don't think we display it anywhere in the UI

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not, let's ask Sergey, maybe he needs it

contracts/superclasses/Posts.sol Outdated Show resolved Hide resolved
contracts/superclasses/Posts.sol Outdated Show resolved Hide resolved
contracts/superclasses/Posts.sol Outdated Show resolved Hide resolved
contracts/models/Reaction.sol Show resolved Hide resolved
contracts/superclasses/Posts.sol Outdated Show resolved Hide resolved
@@ -82,15 +83,20 @@ contract Posts is KetlGuarded {
mapping(uint => mapping(uint => Post[])) public comments;
mapping(uint => mapping(uint => Counters.Counter)) public lastCommentIds;

mapping(uint => mapping(uint => mapping(uint => Reaction[])))
mapping(uint => mapping(uint => mapping(uint => mapping(uint => Reaction))))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is unnecessary

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, right now this map works like:
reactions[feedId][postId][commendId][reactionId]
If commentId === 0, you're accessing the post

Reaction has reactionId inside, if you'll remove something from reaction array, you'll need to update every reactionId
Also you need to make sure on frontend that currentUserReaction is proper (because you removed reaction, array has shifted and you need to shift it on frontend as well)

@@ -234,14 +240,14 @@ contract Posts is KetlGuarded {
uint feedId = commentRequest.feedId;
uint postId = commentRequest.postId;
uint replyTo = commentRequest.replyTo;
// Fetch parent post
Post memory parentPost = posts[feedId][postId];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use this twice below, without this change the git diff would look better

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It didn't work for us for some reason

contracts/superclasses/Posts.sol Outdated Show resolved Hide resolved
contracts/superclasses/Posts.sol Outdated Show resolved Hide resolved
@T-Damer T-Damer merged commit b02ed93 into main Jun 7, 2023
@T-Damer T-Damer deleted the use-postId branch June 7, 2023 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants