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

feat: add indexer related rpc #905

Merged
merged 26 commits into from
Jun 18, 2019
Merged

feat: add indexer related rpc #905

merged 26 commits into from
Jun 18, 2019

Conversation

quake
Copy link
Member

@quake quake commented May 27, 2019

This PR added 5 indexer related rpc, most lines of code are unit / integration test and document, core logic is in indexer/src/store.rs

@quake quake requested a review from a team May 27, 2019 03:32
@nervos-bot
Copy link

nervos-bot bot commented May 27, 2019

@xxuejie is assigned as the chief reviewer

@quake quake requested a review from a team May 27, 2019 03:38
Copy link
Collaborator

@xxuejie xxuejie left a comment

Choose a reason for hiding this comment

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

A more general comment: personally I feel like this module shouldn't belong to CKB core, but right now we don't have a good way to keep it out of core. Should we get the chance to refactor CKB with plugin support, we should move this out of core and only works as a CKB plugin.

db/src/lib.rs Show resolved Hide resolved
notify/src/lib.rs Outdated Show resolved Hide resolved
rpc/README.md Outdated Show resolved Hide resolved
util/jsonrpc-types/src/wallet.rs Outdated Show resolved Hide resolved
#[derive(Serialize, Deserialize)]
pub struct CellTransaction {
pub created_by: TransactionPoint,
pub consumed_by: Option<TransactionPoint>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if the name here makes sense, a transaction consumes cells, not transaction, and yet here we have a transaction that is consumed by many other transactions. CellTransaction is also an interesting name here.

Copy link
Member Author

Choose a reason for hiding this comment

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

open for discussion

wallet/src/store.rs Outdated Show resolved Hide resolved
wallet/src/store.rs Outdated Show resolved Hide resolved
wallet/src/store.rs Outdated Show resolved Hide resolved
wallet/src/store.rs Outdated Show resolved Hide resolved
wallet/src/store.rs Outdated Show resolved Hide resolved
@doitian doitian changed the title feat: add wallet related rpc feat: add indexer related rpc May 27, 2019
@quake
Copy link
Member Author

quake commented May 28, 2019

@xxuejie most review issues have been resolved, others are open for discussion, please review again, thanks.

@quake quake requested review from xxuejie and a team May 28, 2019 08:19
@doitian doitian requested a review from a team May 29, 2019 05:35
indexer/src/store.rs Outdated Show resolved Hide resolved
db/src/lib.rs Show resolved Hide resolved
@doitian doitian changed the title feat: add indexer related rpc ✋ feat: add indexer related rpc Jun 4, 2019
nervos-bot[bot]
nervos-bot bot previously requested changes Jun 4, 2019
Copy link

@nervos-bot nervos-bot bot left a comment

Choose a reason for hiding this comment

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

Hold as requested by @doitian.

@quake quake force-pushed the quake/wallet branch 3 times, most recently from 7b4fd4a to e06ab0b Compare June 8, 2019 01:27
@quake quake changed the title ✋ feat: add indexer related rpc feat: add indexer related rpc Jun 9, 2019
@nervos-bot nervos-bot bot dismissed their stale review June 9, 2019 05:33

Unhold as requested by @quake.

@quake quake requested review from doitian and jjyr June 9, 2019 05:39
indexer/src/store.rs Show resolved Hide resolved
indexer/src/store.rs Outdated Show resolved Hide resolved
indexer/src/store.rs Outdated Show resolved Hide resolved
indexer/src/store.rs Show resolved Hide resolved
indexer/src/store.rs Outdated Show resolved Hide resolved
indexer/src/store.rs Outdated Show resolved Hide resolved
@TheWaWaR
Copy link
Contributor

Conflicted

@doitian
Copy link
Member

doitian commented Jun 17, 2019

How to handle schema migration? The main database stores the version into the database to check compatibility.

@quake
Copy link
Member Author

quake commented Jun 17, 2019

How to handle schema migration? The main database stores the version into the database to check compatibility.

Currently, there are no version checking in the indexer store, I would like to refactor version and corruption checking of rocksdb to an utility function in future PR.

indexer/src/types.rs Show resolved Hide resolved
rpc/json/rpc.json Show resolved Hide resolved
rpc/json/rpc.json Outdated Show resolved Hide resolved
rpc/json/rpc.json Outdated Show resolved Hide resolved
rpc/json/rpc.json Show resolved Hide resolved
util/app-config/src/app_config.rs Outdated Show resolved Hide resolved
rpc/src/module/indexer.rs Show resolved Hide resolved
indexer/src/store.rs Show resolved Hide resolved
indexer/src/store.rs Show resolved Hide resolved
indexer/src/store.rs Show resolved Hide resolved
@quake quake requested a review from doitian June 18, 2019 00:42
@doitian doitian merged commit ec6f5c8 into develop Jun 18, 2019
@doitian doitian deleted the quake/wallet branch June 18, 2019 04:53
This was referenced Jun 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants