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.

Sign up for GitHub

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

Jump to bottom

868-4 - Lorre for Bitcoin #867

Merged
merged 43 commits into from
Jul 10, 2020
Merged

868-4 - Lorre for Bitcoin #867

merged 43 commits into from
Jul 10, 2020

Conversation

molowny
Copy link
Contributor

@molowny molowny commented Jun 29, 2020

No description provided.

@molowny molowny changed the title Lorre for Bitcoin 868-4 - Lorre for Bitcoin Jun 30, 2020
@molowny molowny added the Btc-Int Bitcoin Integration label Jun 30, 2020
ivanopagano
ivanopagano suggested changes Jun 30, 2020
View reviewed changes
Copy link
Contributor

@ivanopagano ivanopagano left a comment

Choose a reason for hiding this comment

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

I left my remarks for you

conseil-lorre/src/main/scala/tech/cryptonomic/conseil/indexer/bitcoin/BitcoinOperations.scala Show resolved Hide resolved
conseil-lorre/src/main/scala/tech/cryptonomic/conseil/indexer/bitcoin/BitcoinOperations.scala Outdated Show resolved Hide resolved
conseil-lorre/src/main/scala/tech/cryptonomic/conseil/indexer/bitcoin/BitcoinOperations.scala Outdated Show resolved Hide resolved
ajuszczak
ajuszczak suggested changes Jul 3, 2020
View reviewed changes
Copy link
Contributor

@ajuszczak ajuszczak left a comment

Choose a reason for hiding this comment

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

Overall looks surprisingly clean to me (I was a bit afraid of that). I left couple of remarks, things to consider. Hope you find them valuable :)

conseil-lorre/src/main/scala/tech/cryptonomic/conseil/indexer/bitcoin/BitcoinIndexer.scala Outdated Show resolved Hide resolved
conseil-lorre/src/main/scala/tech/cryptonomic/conseil/indexer/bitcoin/BitcoinIndexer.scala Outdated Show resolved Hide resolved
conseil-lorre/src/main/scala/tech/cryptonomic/conseil/indexer/bitcoin/BitcoinOperations.scala Outdated Show resolved Hide resolved
conseil-lorre/src/main/scala/tech/cryptonomic/conseil/indexer/bitcoin/BitcoinOperations.scala Outdated Show resolved Hide resolved
docker-compose.yml Show resolved Hide resolved
@ajuszczak
Copy link
Contributor

I see -3% in tests coverage. Is there something we can do about that? I mean, it is quite significant. We should rather go up than down here :D

@molowny molowny force-pushed the lorre-for-bitcoin branch 2 times, most recently from a3ade72 to a34b73b Compare July 3, 2020 14:34
@molowny molowny force-pushed the bitcoin-persistence branch 2 times, most recently from 8f06520 to b2a7307 Compare July 3, 2020 14:43
@molowny molowny changed the base branch from bitcoin-persistence to master July 3, 2020 15:01
ivanopagano
ivanopagano reviewed Jul 3, 2020
View reviewed changes
build.sbt Outdated Show resolved Hide resolved
ivanopagano
ivanopagano reviewed Jul 3, 2020
View reviewed changes
Copy link
Contributor

@ivanopagano ivanopagano left a comment

Choose a reason for hiding this comment

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

still in between, will need to update the review.

It's a shame that force pushing actually loses any comments history

conseil-lorre/src/main/scala/tech/cryptonomic/conseil/indexer/bitcoin/BitcoinIndexer.scala Outdated Show resolved Hide resolved
conseil-lorre/src/main/scala/tech/cryptonomic/conseil/indexer/bitcoin/BitcoinIndexer.scala Outdated Show resolved Hide resolved
conseil-lorre/src/main/scala/tech/cryptonomic/conseil/indexer/bitcoin/BitcoinIndexer.scala Outdated Show resolved Hide resolved
ajuszczak
ajuszczak reviewed Jul 7, 2020
View reviewed changes
Copy link
Contributor

@ajuszczak ajuszczak left a comment

Choose a reason for hiding this comment

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

I've added couple of comments if you could quickly take a look.

build.sbt Outdated Show resolved Hide resolved
conseil-lorre/src/main/scala/tech/cryptonomic/conseil/indexer/bitcoin/BitcoinOperations.scala Outdated Show resolved Hide resolved
conseil-lorre/src/main/scala/tech/cryptonomic/conseil/indexer/bitcoin/BitcoinOperations.scala Outdated Show resolved Hide resolved
...il-lorre/src/test/scala/tech/cryptonomic/conseil/indexer/bitcoin/BitcoinOperationsTest.scala Outdated Show resolved Hide resolved
molowny added 26 commits July 9, 2020 08:49
@molowny
Lorre for Bitcoin
5c60d67
@molowny
Preparation to tests
a7ff981
@molowny
save data into database
5f2282f
@molowny
Persistence fix
256e0a7
@molowny
Bitcoin json model and Slick db
667a4dc
@molowny
Bitcoin persistence with test
c502e4b
@molowny
Working lorre for Bitcoin without tests
506afc4
@molowny
Documentation
e0eed6e
@molowny
Lorre with fixes
288e26c
@molowny
Remarks form the PR
28b7bdc
@molowny
Batching configuration
8667289
@molowny
Remarks from the PR
12d513e
@molowny
Remarks from the PR
e031ace
@molowny
BitcoinOperations test
96e820f
@molowny
Tests for the BitcoinOperations
f4d9fff
@molowny
Remarks from the PR
f2d3d69
@molowny
Methods moved from Bitcoin operations to persistence
a534b92
@molowny
Tests for BitcoinOperations
1c68594
@molowny
Docs fix
6ef6173
@molowny
getIndexedBlocks renamed to getIndexedBlockHeights
d93819e
@molowny
Remarks from the PR
2aa5a5e
@molowny
BitcoinOperationsTest refactor
7a7474d
@molowny
Get rid of common tests dependency in build.sbt
f200dce
@molowny
Remarks from the PR
ad1399a
@molowny
BitcoinPersistence fix
8bbf19d
@molowny
LorreAppConfig fix
905cfbb
@vishakh vishakh merged commit 63c180c into master Jul 10, 2020
@molowny molowny deleted the lorre-for-bitcoin branch July 10, 2020 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajuszczak ajuszczak ajuszczak approved these changes

@piotrkosecki piotrkosecki piotrkosecki approved these changes

@ivanopagano ivanopagano ivanopagano approved these changes

@vishakh vishakh Awaiting requested review from vishakh

Assignees
No one assigned
Labels
Btc-Int Bitcoin Integration
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@molowny @ajuszczak @ivanopagano @piotrkosecki @vishakh