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

Incorporate an optional title parameter into the AddAssetQuantity and SubtractAssetQuantity function for the Python client, aligning with the enhanced functionality within Iroha Core for optimized message communication. #157

Merged
merged 2 commits into from
Feb 7, 2024

Conversation

dominious1
Copy link
Contributor

@dominious1 dominious1 commented Nov 2, 2023

Introduce an optional title parameter to the commands.proto bufs GRPC for AddAssetQuantity and SubtractAssetQuantity. This enhancement facilitates a more flexible and comprehensive feature set, aligning with evolving requirements and ensuring seamless integration with the protobuf-based GRPC implementation.

Technology stack:

  • Ubuntu 22.04.3 LTS
  • Python 3.10
  • PyCharm
  • protoc libprotoc 23.1

To regenerate files based on predefined protobufs I used iroha-python/scripts/compile-proto.py

This is part of internship project:
https://wiki.hyperledger.org/display/INTERN/Iroha+1%3A+extend+queries+with+optional+arguments

Link to PR to iroha repository compatible with those changes:
hyperledger/iroha#4003

Copy link
Member

@baziorek baziorek left a comment

Choose a reason for hiding this comment

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

You changed *.proto file, but You need to generate code. Please execute:

scripts/compile-proto.py

@baziorek baziorek added the iroha1 label Nov 2, 2023
@baziorek baziorek changed the title add title param as optional to AddAssetQuantity add title param as optional to AddAssetQuantity and SubtractAssetQuantity (internship 2023) Nov 2, 2023
@baziorek baziorek changed the title add title param as optional to AddAssetQuantity and SubtractAssetQuantity (internship 2023) add optional title param to AddAssetQuantity and SubtractAssetQuantity (internship 2023) Nov 2, 2023
@baziorek baziorek changed the title add optional title param to AddAssetQuantity and SubtractAssetQuantity (internship 2023) Add optional title param to AddAssetQuantity and SubtractAssetQuantity (internship 2023) Nov 2, 2023
@dominious1
Copy link
Contributor Author

Generated files added after adding new parameter.

@baziorek
Copy link
Member

baziorek commented Nov 3, 2023

Generated files added after adding new parameter.

Great!

As I remember there were some problems with protobuf files when they were generated with too new protobuf. Please provide some information how You generated those files, what is Your OS? What version is of protoc in Your system?

And one more think - please correct DCO:
image
when You click "Details" You will see commands how to fix

Copy link

@andprogrammer andprogrammer left a comment

Choose a reason for hiding this comment

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

This is okay.

Copy link

@andprogrammer andprogrammer left a comment

Choose a reason for hiding this comment

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

This is okay.

@dominious1 dominious1 force-pushed the addtitleparamtoAddAssetQuantity branch from 54d73c1 to 490be39 Compare November 3, 2023 10:42
Copy link
Member

@baziorek baziorek left a comment

Choose a reason for hiding this comment

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

commands.proto looks OK, rest of files are generated, so probably they are also OK. So next thing is to finish: hyperledger/iroha#4003

…new parameters for advanced message communication. Implement corresponding handling capabilities in Iroha Core to ensure seamless integration and optimal performance.

Signed-off-by: dominious1 <[email protected]>
@dominious1 dominious1 force-pushed the addtitleparamtoAddAssetQuantity branch from 490be39 to 7e63a5a Compare November 9, 2023 22:01
@dominious1 dominious1 changed the title Add optional title param to AddAssetQuantity and SubtractAssetQuantity (internship 2023) Incorporate an optional title parameter into the AddAssetQuantity function for the Python client, aligning with the enhanced functionality within Iroha Core for optimized message communication. Nov 9, 2023
@dominious1 dominious1 changed the title Incorporate an optional title parameter into the AddAssetQuantity function for the Python client, aligning with the enhanced functionality within Iroha Core for optimized message communication. Incorporate an optional title parameter into the AddAssetQuantity and SubtractAssetQuantity function for the Python client, aligning with the enhanced functionality within Iroha Core for optimized message communication. Nov 23, 2023
Copy link
Member

@baziorek baziorek left a comment

Choose a reason for hiding this comment

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

Yesterday I was having meeting with @dominious1 and @andprogrammer . We corrected name from title->description and regenerated python files from protobuf with official script(https://github.com/hyperledger/iroha-python/blob/main/scripts/compile-proto.py).

@andprogrammer
Copy link

Fantastic pull request. Thank you.

@baziorek baziorek force-pushed the addtitleparamtoAddAssetQuantity branch from ac79788 to ac4d58f Compare February 4, 2024 11:15
@baziorek
Copy link
Member

baziorek commented Feb 4, 2024

To make sure that everything is working I checked:

  1. I've build Iroha 1 from main (all changes are merged)
  2. I've performed commands to install from the PR locally:
mkdir checking_internship
cd checking_internship
python -m venv $(pwd)/venv
source ./venv/bin/activate
pip3 install git+https://github.com/dominious1/iroha-python.git@addtitleparamtoAddAssetQuantity
  1. Then I run attached code: code.py (code.zip)
    Most important part of the file is running the function:
def add_coin_to_admin_with_description(asset: str, amount='1000.00', desc=''):
    """
    Add provided amount of specific units to admin account
    """
    tx = iroha.transaction([
        iroha.command('AddAssetQuantity',
                      asset_id=asset, amount=amount, description=desc)
    ])
    IrohaCrypto.sign_transaction(tx, ADMIN_PRIVATE_KEY)
    send_transaction_and_print_status(tx)
  1. The output contained those commands (full output in file: fullOputput.txt )"
  transactions {
    payload {
      reduced_payload {
        commands {
          add_asset_quantity {
            asset_id: "coin#domain"
            amount: "102"
            description: "Testing description"
          }
        }
        creator_account_id: "admin@test"
        created_time: 1707045588320
        quorum: 1
      }
    }
    signatures {
      public_key: "313a07e6384776ed95447710d15e59148473ccfc052a681317a72a69f2a49910"
      signature: "7a904afaaa208fdb9ec0695e4c6f6acb3f36578d2af46741c143a9e3726b8de0b1b71fc323bdc0c27fbbdbe939298393476dd5b89d1e390e424004185a8cb101"
    }
  }
  1. I've checked in blocks files - the transaction is available amoungh commands:
jq '.blockV1.payload.transactions[0].payload.reducedPayload' < 0000000000000004
{
  "commands": [
    {
      "addAssetQuantity": {
        "assetId": "coin#domain",
        "amount": "102",
        "description": "Testing description"
      }
    }
  ],
  "creatorAccountId": "admin@test",
  "createdTime": "1707045588320",
  "quorum": 1
}

IMO it is done - fully working!

Copy link
Contributor

@6r1d 6r1d left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for your contribution. I'm concerned that we have blobs inside the Python code, and this is a topic I'd like to discuss separately.

Copy link
Contributor

@6r1d 6r1d left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for your contribution. I'm concerned that we have blobs inside the Python code, and this is a topic I'd like to discuss separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants