refine explorer api #131

dj8yfo · 2017-05-28T02:42:15Z

dj8yfo · 2017-05-29T00:31:27Z

http:://127.0.0.1:8200/api/explorer/v1/transactions/24b2dfd51708ae9fe804e1b05f1cae8dadccbbfed7ec765e1b02b661ff5e6748

{
  "content": {
    "body": {
      "amount": "1000",
      "from": "83ef2ece5a3e0d8e32f3be4c29f4b38981cfdaa6c42e4804e26ff1b42900480c",
      "seed": "14504918002785601018",
      "to": "c0100d8a1e46b01361ed2e18ad1a2c07bb3b70947607a1bc7dace684372cba2e"
    },
    "message_id": 128,
    "network_id": 0,
    "protocol_version": 0,
    "service_id": 128,
    "signature": "7507745e170a55e4353026e664a7c9af8ef2bb2d48f3837fc589f1988c8b98693e3d5819fb0b7be3fd132e8d9b8ab46bb6847c1dd309881f8194f9c0cbb7300e"
  },
  "location": {
    "block_height": "2510",
    "position_in_block": "1"
  },
  "proof_to_block_merkle_root": {
    "left": {
      "left": "e54107aeb6349f5f34436367f9e10639c4503616e34e96de81e376e64093a156",
      "right": {
        "val": "d860dacd76c708fc9d721df734450e7bd9eb6bd5238a68feba03f558b620dcfa"
      }
    },
    "right": "2cf957a684ab28a3edd8c72bc2bfc9568eaf85f238ae1b68b5183e0e4bbed390"
  }
}

dj8yfo · 2017-05-29T00:33:21Z

http://127.0.0.1:8200/api/explorer/v1/blocks/2510

{
  "block": {
    "height": "2510",
    "prev_hash": "9dfda3ca9f0531e20db2e2ae7763942627be4ab41bb6e19805bed3c15c0eed08",
    "propose_round": 1,
    "state_hash": "d9c796a28bcede02d74efaac345cf5cda9ec565f6ab29338e9599188478ada62",
    "tx_hash": "7de38873cd149a4b60abdc490d4c057d20da3b9d95458540221fd01939a83381"
  },
  "precommits": [
    {
      "body": {
        "block_hash": "52142c72dd60b53954aee7432f5bb4bd45adf15cb86fb88461113414045d61d2",
        "height": "2510",
        "propose_hash": "33a08c322f79df3d63ea15439ac6dcb51ff9fe071800c56f8a86c32b2af3c907",
        "round": 1,
        "time": {
          "nanos": 492091000,
          "secs": "1496016744"
        },
        "validator": 3
      },
      "message_id": 4,
      "network_id": 0,
      "protocol_version": 0,
      "service_id": 0,
      "signature": "a6efde647260d5b5fc56db28ec5cb1443ee6c01190e8441f38f6cc574871257aa5f61c360559550ed65c1179c66833ddce59179f3a3e68f8e53debf0e5f0180c"
    },
    {
      "body": {
        "block_hash": "52142c72dd60b53954aee7432f5bb4bd45adf15cb86fb88461113414045d61d2",
        "height": "2510",
        "propose_hash": "33a08c322f79df3d63ea15439ac6dcb51ff9fe071800c56f8a86c32b2af3c907",
        "round": 1,
        "time": {
          "nanos": 492394000,
          "secs": "1496016744"
        },
        "validator": 2
      },
      "message_id": 4,
      "network_id": 0,
      "protocol_version": 0,
      "service_id": 0,
      "signature": "a282655d7cdeb2fa5224e506e911e104e0cc973830728e94b66983308e8d5b674c1b0ad697759e5e2fa8edfd9a64ad6d4832347896a4b7c9e7264b4fb9dc1d0c"
    },
    {
      "body": {
        "block_hash": "52142c72dd60b53954aee7432f5bb4bd45adf15cb86fb88461113414045d61d2",
        "height": "2510",
        "propose_hash": "33a08c322f79df3d63ea15439ac6dcb51ff9fe071800c56f8a86c32b2af3c907",
        "round": 1,
        "time": {
          "nanos": 492533000,
          "secs": "1496016744"
        },
        "validator": 4
      },
      "message_id": 4,
      "network_id": 0,
      "protocol_version": 0,
      "service_id": 0,
      "signature": "9bad053eb6926139558304d7d07edc3b6cbb19a417e5c12cba171026d6b5411762b242030feba7950a4d3252e66923fbe5bbb5a5173a514fce12889ab59f3306"
    },
    {
      "body": {
        "block_hash": "52142c72dd60b53954aee7432f5bb4bd45adf15cb86fb88461113414045d61d2",
        "height": "2510",
        "propose_hash": "33a08c322f79df3d63ea15439ac6dcb51ff9fe071800c56f8a86c32b2af3c907",
        "round": 1,
        "time": {
          "nanos": 667184000,
          "secs": "1496016744"
        },
        "validator": 1
      },
      "message_id": 4,
      "network_id": 0,
      "protocol_version": 0,
      "service_id": 0,
      "signature": "976715d3f744dbf7a226eca0211dd5d1df864551bd8ccdbd9a6301f44a331569bbf9effcdcb9d36670b689cc2d7b44fa0d2bb9025979a17c60e1baf3ed8bff09"
    },
    {
      "body": {
        "block_hash": "52142c72dd60b53954aee7432f5bb4bd45adf15cb86fb88461113414045d61d2",
        "height": "2510",
        "propose_hash": "33a08c322f79df3d63ea15439ac6dcb51ff9fe071800c56f8a86c32b2af3c907",
        "round": 1,
        "time": {
          "nanos": 691582000,
          "secs": "1496016744"
        },
        "validator": 0
      },
      "message_id": 4,
      "network_id": 0,
      "protocol_version": 0,
      "service_id": 0,
      "signature": "bbb12b7c88ab44ca527c2d0a19b0818693a27b417ca6cf17b90fb7d58720f03b397463c71040ce6c1b95f8349adcec1ab54696ea3c8113305195650256f6fb0e"
    }
  ],
  "txs": [
    "24b2dfd51708ae9fe804e1b05f1cae8dadccbbfed7ec765e1b02b661ff5e6748",
    "d860dacd76c708fc9d721df734450e7bd9eb6bd5238a68feba03f558b620dcfa",
    "e6a73e851fe5638d68d2eff1a9bde00e5b5d860a50fd61aaae5f3a6cfdf185ee",
    "e895d486772bddd8ad54dac3d49a724445c23708ce4d4e627c490ceef35b59d0"
  ]
}

dj8yfo · 2017-05-29T16:14:24Z

exonum/src/api/mod.rs

@@ -44,6 +45,7 @@ impl ::std::fmt::Display for ApiError {
 impl ::std::error::Error for ApiError {
    fn description(&self) -> &str {
        match *self {
+            ApiError::Service(ref string) => string,


as per @defuz request , not deepening on detailing and making errors a bit more standardized. Not to invest much time now. Or ever?

dj8yfo · 2017-05-29T16:15:30Z

As per @defuz request no tests for explorer-api, seems to be working fine after testing manually.

dj8yfo · 2017-05-29T16:17:23Z

http://127.0.0.1:8200/api/explorer/v1/blocks?count=950&from=3000&skip_empty_blocks=true

[
  {
    "height": "2510",
    "prev_hash": "9dfda3ca9f0531e20db2e2ae7763942627be4ab41bb6e19805bed3c15c0eed08",
    "propose_round": 1,
    "state_hash": "d9c796a28bcede02d74efaac345cf5cda9ec565f6ab29338e9599188478ada62",
    "tx_hash": "7de38873cd149a4b60abdc490d4c057d20da3b9d95458540221fd01939a83381"
  },
  {
    "height": "2456",
    "prev_hash": "727492e5b5d7d436e556fbe55bfdb0b185250664dc21e49b7f1582f34d35b179",
    "propose_round": 1,
    "state_hash": "45532a288947644950486decb48399d9262664b81231eeb373c443aa3321cb01",
    "tx_hash": "afa42174c6122d05bf7501ff31db38375c0a7e06bbafcebc7b28511bf6b95bc1"
  }
]

stanislav-tkach

I have one concern about skip_empty_blocks: bool flag is not very clear, especially on the call site. Can we split get_blocks function into two?
I'm even remember a similar discussion in some other pull request...

stanislav-tkach · 2017-05-30T08:00:19Z

exonum/src/explorer/explorer_api.rs

@@ -74,11 +70,14 @@ impl Api for ExplorerApi {
                }
                _ => None,
            };
+            skip_empty_blocks = match map.find(&["skip_empty_blocks"]) {
+                Some(&Value::String(ref skip_str)) => {


Is there any reason not to declare skip_empty_blocks here?

@DarkEld3r what specific pr do you imply?

For this comment: let skip_empty_blocks = match map.find(....

slowli · 2017-05-30T10:12:46Z

exonum/src/explorer/mod.rs

@@ -91,22 +91,28 @@ impl<'a> BlockchainExplorer<'a> {
        Ok(res)
    }

-    pub fn blocks_range(&self, count: u64, from: Option<u64>) -> StorageResult<Vec<BlockInfo>> {
+    pub fn blocks_range(&self, count: u64, upper: Option<u64>, skip_empty_blocks: bool) -> StorageResult<Vec<Block>> {


What's the reasoning for skip_empty_blocks request param? If I understand correctly, it seems to harm security because with some blocks skipped, it becomes impossible to check the integrity of the blockchain (the fact that each block references the previous one, i.e., commits to the entire previous state of the blockchain).

If the reasoning is skipping downloading some blocks for lightweight clients, skip lists (e.g., referencing not only the prev block, but say, the nearest block at a height divisible by 100) seems like a more versatile decision. For example, skip lists allow to skip non-empty blocks; and it's possible to build hierarchical skip lists in order to compact the data returned to lightweight clients even more. OTOH, there are the same security issues with skip lists as with skipping empty blocks; they increase the necessary trust to third parties from the client's side.

@slowli yes, it's for client. for ui only. we'll make a separate pr for indexing nonempty blocks and then actually returning last n nonempty blocks instead of nonempty blocks among last n blocks.i'll look into the skip lists, but i assume it's a heavier change, involving storage and protocol and not local to api only.

dj8yfo · 2017-05-30T14:14:30Z

@DarkEld3r

get_blocks function into two?

how to split and what for?

stanislav-tkach · 2017-05-30T14:48:03Z

OK, now I see that it will be not so easy, so it is probably not worth it.

stanislav-tkach · 2017-05-31T07:54:52Z

One more time on that. 😄 First of all, I don't insist on that, but as for me, unnamed bool flags in API look not very good. When you see blocks_range(..., false) call you need to check function declaration in order to understand what that false means.

So this can look the following way:

fn blocks(&self, count: u64, upper: Option<u64>) -> StorageResult<Vec<Block>> {
    // Current implementation without skipping empty blocks.
    // ...
}

fn non_empty_blocks(&self, count: u64, upper: Option<u64>) -> StorageResult<Vec<Block>> {
    let blocks = blocks()?;
    let schema = Schema::new(&view);
    blocks.filter(|block| schema.block_txs(block.height()).is_empty()).collect();
}

Then again, probably that solution is suboptimal, so I'm not insisting. At second thought, it will probably not be used in performance critical parts. Plus implementation can be updated after #76 is closed.

@defuz @slowli I would like to hear other opinions.

alekseysidorov · 2017-05-31T10:04:07Z

exonum/src/api/mod.rs

@@ -23,6 +23,7 @@ use storage::{Result as StorageResult, Error as StorageError};

 #[derive(Debug)]
 pub enum ApiError {
+    Service(String),


Why not Box<StdError>?

alekseysidorov · 2017-05-31T10:05:31Z

exonum/src/explorer/explorer_api.rs

-    pub from: Option<u64>,
-    pub count: u64,
-}
+const MAX_BLOCKS_PER_REQUEST: u64 = 1000;


Is not better to give the ability to set this variable via configuration?

alekseysidorov · 2017-05-31T10:07:52Z

exonum/src/explorer/explorer_api.rs

-            Err(e) => Err(ApiError::Storage(e)),
+    fn get_blocks(&self, count: u64, from: Option<u64>, skip_empty_blocks: bool) -> Result<Vec<Block>, ApiError> {
+        if count > MAX_BLOCKS_PER_REQUEST {
+             return Err(ApiError::IncorrectRequest)


It seems that the IncorrectRequest need to have a short description why the error occured. Is it possible in http responses?

dj8yfo · 2017-05-31T15:04:17Z

@deniskolodin we could transfer to using error_chain instead of these poor crutches. But that has to be performed consistently over whole exonum-core. I propose merging this as is, and then handling all core errors in separate pr.

therustmonk · 2017-05-31T15:17:26Z

@gisochre I agree that's better to change error handling in a separate PR and I'm even ready to propose one after you merge these changes. I believe that the code will be easier to maintain.

dj8yfo · 2017-05-31T21:45:45Z

{
  "debug": "FromHex(InvalidHexLength)",
  "description": "invalid length"
}

{
  "debug": "IncorrectRequest(ParseIntError { kind: InvalidDigit })",
  "description": "invalid digit found in string"
}

{
  "debug": "IncorrectRequest(BodyError { detail: \"Can\\'t parse body to the struct\", cause: JsonError(ErrorImpl { code: Message(\"Can\\'t parse TxCreateWallet ErrorImpl { code: Message(\\\"Can not deserialize value.\\\"), line: 0, column: 0 }\"), line: 0, column: 0 }) })",
  "description": "Can't parse body to the struct"
}

vldm · 2017-06-01T08:36:14Z

exonum/src/api/mod.rs

            ApiError::FileNotFound(_) => "FileNotFound",
            ApiError::NotFound => "NotFound",
-            ApiError::FileToBig => "FileToBig",
+            ApiError::FileTooBig => "FileToBig",


Update info too

I suppose error descriptions should be slightly different: "File not found" instead of "FileNotFound", etc.

vldm · 2017-06-01T08:44:52Z

exonum/src/api/mod.rs

@@ -87,7 +89,8 @@ impl From<ApiError> for IronError {
        use std::error::Error;

        let mut body = BTreeMap::new();
-        body.insert("type", e.description().into());
+        body.insert("debug", format!("{:?}", e));


If it debug, may be we should control output with some variables?
Or give another name, just to not scare user?

@vldm what you mean?

stanislav-tkach · 2017-06-05T19:01:44Z

exonum/src/api/mod.rs

            ApiError::FileNotFound(_) => "FileNotFound",
            ApiError::NotFound => "NotFound",
-            ApiError::FileToBig => "FileToBig",
+            ApiError::FileTooBig => "FileToBig",


I suppose error descriptions should be slightly different: "File not found" instead of "FileNotFound", etc.

stanislav-tkach · 2017-06-05T19:04:40Z

exonum/src/explorer/explorer_api.rs

                }
                _ => None,
            };
+            let skip_empty_blocks: bool = match map.find(&["skip_empty_blocks"]) {


Probably, type declaration isn't needed here.

dj8yfo · 2017-06-06T00:05:32Z

exonum/exonum-configuration#31
exonum/cryptocurrency#28

defuz · 2017-06-06T17:02:13Z

It was decided to merge it AS IS, but need to be improved in separate PR (depends on #60). @gisochre

Inherit Transaction from Message Closes #131 See merge request !98 Former-commit-id: b1ff1fdb61149eb968d875c25b6e2ef9dfb84e9f

refine explorer api Former-commit-id: 9107f2e7d5fb370a219c16d58608e24e404f330c

Inherit Transaction from Message Closes exonum#131 See merge request !98

block by height request

b5a3457

dj8yfo added this to the Release 0.1 milestone May 29, 2017

Oleg Kuzmin added 2 commits May 29, 2017 03:48

Added info on endpoint 'transaction/:hash'

c0862f7

add 'skip_empty_blocks' to blocks request. Restict on 'count' param

417949d

dj8yfo changed the title ~~WIP: refine explorer api~~ refine explorer api May 29, 2017

dj8yfo commented May 29, 2017

View reviewed changes

dj8yfo requested review from alekseysidorov, defuz and vldm May 29, 2017 16:15

dj8yfo requested a review from stanislav-tkach May 29, 2017 16:18

stanislav-tkach previously approved these changes May 30, 2017

View reviewed changes

slowli reviewed May 30, 2017

View reviewed changes

Minor edit

71dffc4

dj8yfo dismissed stanislav-tkach’s stale review via 71dffc4 May 30, 2017 13:46

stanislav-tkach previously approved these changes May 30, 2017

View reviewed changes

alekseysidorov reviewed May 31, 2017

View reviewed changes

dj8yfo added API breaking change ⚠️ API or ABI is changed. labels May 31, 2017

dj8yfo mentioned this pull request May 31, 2017

Add txs_length field to Block #76 #140

Closed

Basic detailing on api errors added.

4bffb1f

dj8yfo dismissed stanislav-tkach’s stale review via 4bffb1f May 31, 2017 14:44

Fix clippy linted warning

e6bcce5

Error body layout in response

1d4a465

vldm previously approved these changes Jun 1, 2017

View reviewed changes

Remove panic in api thread if no matching service found for tx in db

1caa486

dj8yfo dismissed vldm’s stale review via 1caa486 June 1, 2017 22:29

dj8yfo requested review from alekseysidorov, stanislav-tkach and vldm June 5, 2017 15:32

stanislav-tkach approved these changes Jun 5, 2017

View reviewed changes

defuz mentioned this pull request Jun 6, 2017

Managing node through managing API ("RPC") #60

Closed

19 tasks

defuz approved these changes Jun 6, 2017

View reviewed changes

defuz merged commit 7c3d449 into exonum:master Jun 6, 2017

defuz deleted the refine-explorer-api branch June 6, 2017 17:02

This was referenced Jun 6, 2017

improve blockchain_explorer's get endpoints' layout #108

Closed

Add proof of tx inclusion within block in blockchain_explorer #78

Closed

vldm pushed a commit that referenced this pull request Jul 13, 2017

Merge branch '131-extensions-polish' into 'master'

99f4017

Inherit Transaction from Message Closes #131 See merge request !98 Former-commit-id: b1ff1fdb61149eb968d875c25b6e2ef9dfb84e9f

vldm pushed a commit that referenced this pull request Jul 13, 2017

Merge pull request #131 from gisochre/refine-explorer-api

e5d4adb

refine explorer api Former-commit-id: 9107f2e7d5fb370a219c16d58608e24e404f330c

stanislav-tkach pushed a commit to stanislav-tkach/exonum that referenced this pull request May 18, 2018

Merge branch '131-extensions-polish' into 'master'

832daeb

Inherit Transaction from Message Closes exonum#131 See merge request !98

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refine explorer api #131

refine explorer api #131

dj8yfo commented May 28, 2017 •

edited

dj8yfo commented May 29, 2017 •

edited

dj8yfo commented May 29, 2017 •

edited

dj8yfo May 29, 2017

dj8yfo commented May 29, 2017

dj8yfo commented May 29, 2017

stanislav-tkach left a comment

stanislav-tkach May 30, 2017

dj8yfo May 30, 2017

stanislav-tkach May 30, 2017

slowli May 30, 2017

dj8yfo May 30, 2017

dj8yfo commented May 30, 2017

stanislav-tkach commented May 30, 2017

stanislav-tkach commented May 31, 2017

alekseysidorov May 31, 2017

alekseysidorov May 31, 2017

alekseysidorov May 31, 2017

dj8yfo commented May 31, 2017

therustmonk commented May 31, 2017

dj8yfo commented May 31, 2017 •

edited

vldm Jun 1, 2017

stanislav-tkach Jun 5, 2017

vldm Jun 1, 2017

dj8yfo Jun 5, 2017

stanislav-tkach Jun 5, 2017

stanislav-tkach Jun 5, 2017

dj8yfo commented Jun 6, 2017

defuz commented Jun 6, 2017

refine explorer api #131

refine explorer api #131

Conversation

dj8yfo commented May 28, 2017 • edited

dj8yfo commented May 29, 2017 • edited

dj8yfo commented May 29, 2017 • edited

Choose a reason for hiding this comment

dj8yfo commented May 29, 2017

dj8yfo commented May 29, 2017

stanislav-tkach 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

dj8yfo commented May 30, 2017

stanislav-tkach commented May 30, 2017

stanislav-tkach commented May 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dj8yfo commented May 31, 2017

therustmonk commented May 31, 2017

dj8yfo commented May 31, 2017 • edited

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

dj8yfo commented Jun 6, 2017

defuz commented Jun 6, 2017

dj8yfo commented May 28, 2017 •

edited

dj8yfo commented May 29, 2017 •

edited

dj8yfo commented May 29, 2017 •

edited

dj8yfo commented May 31, 2017 •

edited