[FIX #185] Readd bounty label by vitvly · Pull Request #235 · status-im/open-bounty

vitvly · 2018-01-25T08:44:21Z

FIX #185

Description

This fix does the following:

Listen to unlabeled events from GitHub API.
If such event is received, find the associated issue in the DB and remove it only if funds are zero (currently it checks the field value_usd field). This removes the bounty from SOB.
If it is successfully deleted from the DB, post to GitHub API a request to delete a comment. This will remove a bounty comment on GitHub.
No actions on part of the contract itself are done.

Questions

Is value_usd the correct field to check against when removing a bounty?
Should anything be done with regards to the bounty contract when the parent comment is deleted? If so, what messages should be sent?

Status: Finished.

vitvly · 2018-01-30T16:16:12Z

Changed the comment deletion logic in the DB so that it moves a deleted record to issues_archived table instead of dropping it altogether.

pablodip · 2018-01-31T10:16:04Z

Not sure about the current strategy, since having a similar table creates dependency on the original one, so when we want to modify/split the original table we would also have to do it with the archived table, and in the archived data.

The current goal is to just do not lose information, so I agree with the archive concept, just how about implementing it with a more general approach, for example an archive table with four columns, id, type, archived_at and data, and data could just be the edn of the data that we want to archive? That way it would be more open to archive anything, and we would not have the dependency with the info that we want to archive, so for example the archive would work with any version of the original data. The archived data would not be queriable from SQL, but we would need to take it from there and query it in Clojure, but it is an archive in the end.

Anyway if we seriously aim to not lose information and query better all data over time, that is, to have a time model for the data, we should probably consider using Datomic in the future.

oskarth

Some general comments, no strong opinions on implementation

oskarth · 2018-02-05T15:36:29Z

resources/migrations/20180130164001-issues-archived.down.sql

@@ -0,0 +1 @@
+DROP TABLE issues_archived;


Seems safer to rename it and leave it be for a while, then do cleanup task once it is 100% it works well in both test and production.

This is just a migration down, so why is the alter table really needed? We are not migrating down in production.

I'm not decided on this. Suggest to keep it as is, might help during testing on develop.

It's just because this breaks migration semantics I believe. To me migrations should be able to run up and down several times, and this would break that.

Got you. Sounds reasonable. Changed to DROP.

oskarth · 2018-02-05T15:37:36Z

src/clj/commiteth/routes/webhooks.clj

-    (when (= "reopened" action)
-      (handle-issue-reopened webhook-payload)))
-  (ok))
+    (case action


default case?

use archived issues as a contract pool when deploying new contracts

vitvly · 2018-02-06T14:07:15Z

Changed the code so that deleted issues go to archive table. When deploying new contracts, this table is checked to fetch free contracts that haven't been assigned to any issue. So it fulfills a double purpose: that of an archive and a contract pool.

pablodip · 2018-02-08T10:56:21Z

resources/migrations/20180130164001-archive.up.sql

+        ('user');
+
+CREATE TABLE archive (
+  type TEXT REFERENCES data_types(name),


How about using an enum type so that we are not coupled to database values, but just to structure?

Great idea, changed to enums.

pablodip · 2018-02-08T11:11:49Z

resources/sql/queries.sql

+-- :name remove-issue! :<! :1
+-- :doc removes issue
+WITH archived AS (
+  INSERT INTO archive(type,data)


How about describing this in clojure? I think that it is better to keep persistence as simple as possible and to have all of the logic in the the application code.

I think it's simple enough to stay in a query, plus one roundtrip less to DB is a good thing.

pablodip · 2018-02-08T11:24:46Z

resources/sql/queries.sql

+
+-- :name contract-from-pool :? :1
+-- :doc return first contract from archive for given owner
+SELECT a.data->>'transaction_hash' as transaction_hash


Not totally convinced about treating an archive to use application logic there apart from archiving. How about modelling the Ethereum contract in a table, even if only with the params that we need right now (I think that transaction_hash and owner_address), and we don't need to archive it, but just need an attribute for status?

@siphiuel Let me understand well the query before elaborating the proposal. Why are we selecting a.data->>'transaction_hash' and then filtering for it to be null with a.data->>'transaction_hash' = i.transaction_hash and i.transaction_hash is null? I might be understanding badly the logic. :S

We use left join condition issues.transaction_hash=null so that to pick transaction hashes that are not in use already. It might happen that a record has been removed from issues table to the archive, and then restored. However, the archive table is append-only, so this will result in the same transaction hash being present both in issues and in the archive, and we want to filter that away.

Okay, got it, thanks!

So my proposal was about starting modelling the Ethereum contract, since it is a model with some data in the end. And doing so only with the attributes and behaviours of the contract that we need for this.

For example, we could have a table ethereum_contract, with attrs id, owner_address, transaction_hash, status. When an issue is archived and has a contract that we can reuse, we can create a contract model there with the status not-used (or maybe a better name). Then when looking for available contracts, we search there, and when taking one, we change the status of it to used. That way we would avoid querying the archive table for application logic, and would express more directly the status of a contract and its filtering.

Let's handle contract modeling in a separate issue: #267.

pablodip · 2018-02-08T11:27:05Z

src/clj/commiteth/bounties.clj

      (log/debug "Total issues for repo limit reached " repo " " count)
      (add-bounty-for-issue repo repo-id issue))))

+(defn remove-bounty-for-issue [repo repo-id issue]


Probably better ending with !

pablodip · 2018-02-08T11:27:11Z

src/clj/commiteth/db/issues.clj

                              :issue_number issue-number
                              :title        issue-title})))

+(defn remove


Probably better ending with !

pablodip · 2018-02-08T11:27:22Z

src/clj/commiteth/github/core.clj

    (log/debug "Posting comment to" (str owner "/" repo "/" issue-number) ":" comment)
    (issues/create-comment owner repo issue-number comment (self-auth-params))))

+(defn remove-deploying-comment


Probably better ending with !

use enum instead of table for data type storage

pablodip

It is okay to me.

I think that it would be nice to test it in a clean develop, since it includes database migrations and would be more difficult to revert. @siphiuel What do you think?

vitvly · 2018-02-15T09:15:31Z

Agree.

martinklepsch · 2018-02-20T17:27:01Z

This seems good but I'm also a little bit worried about introducing a separate table. Wouldn't something like revoked_at as suggested by @pablodip be easier to work with?

Thinking about #284 we'd need to

read the JSON from the archive and parse that
handle old versions as the table schema might change over time but the JSON blob stays the same

Are there any specific benefits with the separate table approach?

vitvly · 2018-02-20T18:03:43Z

Good point! Initially I've opted for archive simply because it was a less intrusive code change and did not imply changing multiple queries and associated middleware, hence smaller probability of regressions. Smart vocabulary aside, I was plain lazy:)

Still, can think of 2 things in favor of archive approach:

We'd like to have a DB model that works in an append-only fashion. No data can be deleted without a trace, so that we can recover what happened to each issue and when (audit trail capability). Currently the DB is append-only with regards to issues and pull requests - data is always appended. The archive can record multiple deletions of a single issue's bounty, while revoked_at will record the most recent one.
Related to a concern that archive serves a double purpose: @pablodip suggested modeling contracts in a ethereum_contract table, and changing issues to include a relation to ethereum_contract (Ethereum contract model #267). This would relieve archive of non-archival related duties.

Actually, the whole audit-trail related thing is important and requires more thought.

pablodip · 2018-02-20T20:39:39Z

To me the reason to not face re-modelling here isn't just about laziness, but about strategy. Introducing new tables or fields requires extra work in some existing things, and so might produce bugs as well. And we already intend to re-model with a different structure. So facing re-modelling here and in the new structure isn't probably needed. Besides, the feature that depends on the archive is an optional feature (re-use a contract) and easily refactored if needed.

martinklepsch · 2018-02-20T23:33:48Z

Actually, the whole audit-trail related thing is important and requires more thought.

Agree. Event Sourcing may be an interesting approach for this as well and is fundamentally similar to how Datomic works.
Given the complexities around this issue my feeling is that we should probably "ignore it" for now and later on allocate some time to think about how to implement it properly.

pablodip · 2018-02-21T06:21:04Z

Agree. Event Sourcing may be an interesting approach for this as well and is fundamentally similar to how Datomic works.

I personally don't think they're similar, since Event Sourcing is just about modelling and persisting round domain events, while Datomic is a general-purpose database that adds the time dimension.

churik · 2018-03-14T09:49:23Z

1. GH comment is not posted after deploying contract

Steps:
Requirements: GH account is whitelisted, signed app, test application is added to repo;

navigate to repo
create an issue
add bounty label
open https://testing.openbounty.status.im/app
wait until new bounty will be displayed in 'Open bounties' list
click on bounty

Actual result:

GH comment with contract details

Expected result:

no GH comment

Video: http://take.ms/SMVwj
OS: Mac OS High Sierra
Browser: Chrome 64

…-bounty into feature/readd-bounty-#185

[FIX #185] Add handling for GitHub "unlabeled" actions

8311105

vitvly force-pushed the develop branch 2 times, most recently from 2b88442 to 2f24317 Compare January 26, 2018 16:38

Move deleted issue comments to issues_archived

aed390a

vitvly requested review from oskarth and pablodip January 30, 2018 16:22

vitvly self-assigned this Jan 30, 2018

oskarth reviewed Feb 5, 2018

View reviewed changes

Vitaliy Vlasov added 3 commits February 6, 2018 15:59

Add data archive;

0cb33d8

use archived issues as a contract pool when deploying new contracts

Add default case in issue handling fn

8139d95

Rename archive table instead of dropping it

08049fe

Use DROP instead of RENAME for archive

23b532e

pablodip reviewed Feb 8, 2018

View reviewed changes

Add trailing '!' to 'remove' fns;

85bf4ad

use enum instead of table for data type storage

vitvly mentioned this pull request Feb 14, 2018

Bounty revocation: UI + backend #284

Closed

pablodip requested a review from martinklepsch February 14, 2018 11:21

pablodip approved these changes Feb 15, 2018

View reviewed changes

Merge branch 'develop' into feature/readd-bounty-#185

0991c47

martinklepsch mentioned this pull request Feb 21, 2018

Derive bounty state from database #300

Closed

Merge branch 'develop' into feature/readd-bounty-#185

cf2579e

oskarth approved these changes Feb 22, 2018

View reviewed changes

Merge branch 'develop' into feature/readd-bounty-#185

d6c311b

churik added the Tested-Issues label Mar 14, 2018

Vitaliy Vlasov added 4 commits March 19, 2018 17:04

Merge branch 'develop' into feature/readd-bounty-#185

d74741e

Merge branch 'feature/readd-bounty-#185' of github.com:status-im/open…

754136b

…-bounty into feature/readd-bounty-#185

Merge branch 'develop' into feature/readd-bounty-#185

8bb9f7c

Merge branch 'feature/readd-bounty-#185' of github.com:status-im/open…

a94d67c

…-bounty into feature/readd-bounty-#185

Conversation

vitvly commented Jan 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Questions

Uh oh!

vitvly commented Jan 30, 2018

Uh oh!

pablodip commented Jan 31, 2018

Uh oh!

oskarth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vitvly commented Feb 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pablodip left a comment

Choose a reason for hiding this comment

Uh oh!

vitvly commented Feb 15, 2018

Uh oh!

martinklepsch commented Feb 20, 2018

Uh oh!

vitvly commented Feb 20, 2018

Uh oh!

pablodip commented Feb 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martinklepsch commented Feb 20, 2018

Uh oh!

pablodip commented Feb 21, 2018

Uh oh!

churik commented Mar 14, 2018

1. GH comment is not posted after deploying contract

Actual result:

Expected result:

Uh oh!

vitvly commented Jan 25, 2018 •

edited

Loading

pablodip commented Feb 20, 2018 •

edited

Loading