Skip to content
This repository was archived by the owner on Jan 18, 2019. It is now read-only.

Conversation

@grimadas
Copy link

@grimadas grimadas commented Feb 2, 2018

lebdron
lebdron previously requested changes Feb 6, 2018
Copy link

@lebdron lebdron left a comment

Choose a reason for hiding this comment

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

Please run validator on examples.

```
```json
{
/* Transaction */
Copy link

Choose a reason for hiding this comment

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

Such comments are not valid JSON syntax, consider removing.

"created_ts": 1517560129182,
"creator_account_id": "admin@test",
"tx_counter": 1,
"commands": []
Copy link

Choose a reason for hiding this comment

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

Transactions with no commands are currently invalid, consider adding a small command, such as AddPeer.

{
"command_type": string(?),
/* other command-specific fields */
/* command-specific fields */
Copy link

Choose a reason for hiding this comment

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

Such comments are invalid JSON syntax. Also empty commands are not currently accepted, consider adding a small command such as AddPeer.

Copy link
Author

Choose a reason for hiding this comment

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

The example for the valid transaction is provided above, this is used for explanation. Is it necessary to complicate it, as it might hurt the explanation?

Copy link

Choose a reason for hiding this comment

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

Concern is that person can just try to copy and paste the example and try to run it with iroha-cli. Maybe this explanation can be done in text, and not in JSON?

{
"pubkey": string(64),
"signature": string(128),
"pubkey": "407e57f50ca48969b08ba948171bb2435e035d82cec417e18e4a38f5fb113f83",
Copy link

Choose a reason for hiding this comment

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

Space looks redundant, maybe run some JSON beautifier on examples?

Copy link
Author

Choose a reason for hiding this comment

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

thanks!

"query_counter": 1,
"query_type" : "GetTransactions",
"tx_hashes": [string(64),…]
"tx_hashes": [bf6edb882d53f5532cb416455834878db7af08fb814f8c95d6867a6d9eea4057]
Copy link

Choose a reason for hiding this comment

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

Not sure if this is valid, since other hex strings are enclosed in "".

@grimadas grimadas requested review from luckychess and neewy February 13, 2018 04:17
@lebdron lebdron dismissed their stale review February 13, 2018 09:31

Outdated

@grimadas grimadas requested a review from l4l February 16, 2018 06:44
Copy link

@l4l l4l left a comment

Choose a reason for hiding this comment

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

GetAccountDetail query is mssing
Check the rest details of all types, there's also a lot outdated info here
Also uppercase letters allowed only at the json's keys

"command_type": "DetachRole",
"account_id": "test@test",
"role_name": "user"
"role_name": "User"
Copy link

Choose a reason for hiding this comment

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

Role should be in lower case

"permissions": [
"CanAddAssetQuantity",
"CanAddAssetQuantity"
Copy link

Choose a reason for hiding this comment

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

It was changed to can_add_asset_quantity at hyperledger-iroha/iroha@999a1c7

"pubkey": "",
"signature": ""
"pubkey": "407e57f50ca48969b08ba948171bb2435e035d82cec417e18e4a38f5fb113f83",
"signature": "81744e004555970ad114a2b8f7a0d1bb087e26c6e009a6147781a5042dbbf8e00f1fd5a4d4ddb123c1c0813f00d633b7295e482a43001edbe7f51dd4d32aef05"
Copy link

Choose a reason for hiding this comment

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

Why all of the signatures are the same?

Copy link
Contributor

Choose a reason for hiding this comment

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

this is just an example, don't worry

"query_counter": 1,
"query_type" : "GetRolePermissions",
"role_id" : "MoneyCreator",
"role_id" : "MoneyCreator"
Copy link

Choose a reason for hiding this comment

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

Again role

Signed-off-by: grimadas <[email protected]>
"command_type": "GrantPermission",
"account_id": "takemiya@soramitsu",
"permission_name": "CanAddAssetQuantity"
"permission_name": "can_add_asset_quantity"
Copy link

Choose a reason for hiding this comment

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

can_add_asset_qty :(

"command_type": "AppendRole",
"account_id": "takemiya@test",
"role_name": "Administrator"
"role_name": "administrator"
Copy link

Choose a reason for hiding this comment

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

Seems it too long

"command_type": "RevokePermission",
"account_id": "takemiya@soramitsu",
"permission_name": "CanAddAssetQuantity"
"permission_name": "can_add_asset_quantity"
Copy link

Choose a reason for hiding this comment

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

same

"commands":[
{
"command_type":"CreateAccount",
"account_name":"makoto.takemiya",
Copy link

Choose a reason for hiding this comment

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

'.' isn't accepted

Signed-off-by: grimadas <[email protected]>
Signed-off-by: grimadas <[email protected]>
@l4l
Copy link

l4l commented Mar 24, 2018

Any progress on that?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants