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

Fix examples, add valid keys case #56

Open
wants to merge 9 commits into
base: docs
Choose a base branch
from

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.

@@ -11,11 +11,16 @@ message Transaction {
```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.

@@ -53,13 +58,12 @@ message Payload {
{
"commands": [
{
"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?

@@ -75,10 +79,10 @@ message Signature {
{
"signatures": [
{
"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!

"creator_account_id": "admin@test",
"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 "".

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

@@ -357,9 +356,9 @@ message DetachRole {
{
"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

@@ -319,11 +319,10 @@ message CreateRole {
"command_type": "CreateRole",
"role_name": "MoneyCreator",
"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

"creator_account_id": "admin@test",
"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]>
@@ -393,9 +392,9 @@ message GrantPermission {
{
"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 :(

@@ -164,9 +164,9 @@ message AppendRole {
{
"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

@@ -469,9 +468,9 @@ message RevokePermission {
{
"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

@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