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

Changes in Endorser Protocol #1134

Merged

Conversation

HarshMultani-AyanWorks
Copy link
Contributor

This is a WIP pull request, and it covers the changes in endorser protocol for issues defined in

  1. [Endorser Protocol] Provide an option for the Endorser to write the ledger transaction #1031
  2. [Endorser Protocol] Review Writing Non Secrets #1029

HarshMultani-AyanWorks and others added 2 commits May 2, 2021 02:39
Taking latest changes done till EOD 01/05/2021
…o save record in author's wallet.

Signed-off-by: Harsh Multani <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented May 2, 2021

Codecov Report

Merging #1134 (8130e52) into main (e208697) will decrease coverage by 0.18%.
The diff coverage is 67.15%.

@@            Coverage Diff             @@
##             main    #1134      +/-   ##
==========================================
- Coverage   98.68%   98.50%   -0.19%     
==========================================
  Files         450      452       +2     
  Lines       24784    24855      +71     
==========================================
+ Hits        24459    24484      +25     
- Misses        325      371      +46     

@ianco
Copy link
Contributor

ianco commented May 4, 2021

Hi @HarshMultani-AyanWorks , it's hard to tell from the code, but the default behaviour should be that the Author writes the transaction to the ledger and updates the schema/cred def non-secrets record to their own wallet. There should be an option (maybe a new parameter in the create-request endpoint) to ask the Endorser to write the ledger transaction (in this scenario the Author still needs to write the non-secrets record to their wallet). It looks like this update moves all of this functionality to the Endorser? (Unless I'm not reading the code properly?) Let me know once you've updated the integration test, it's broken now so difficult to review ...

Signed-off-by: Harsh Multani <[email protected]>
sklump
sklump previously approved these changes May 7, 2021
Signed-off-by: Harsh Multani <[email protected]>
@HarshMultani-AyanWorks
Copy link
Contributor Author

Hi @HarshMultani-AyanWorks , it's hard to tell from the code, but the default behaviour should be that the Author writes the transaction to the ledger and updates the schema/cred def non-secrets record to their own wallet. There should be an option (maybe a new parameter in the create-request endpoint) to ask the Endorser to write the ledger transaction (in this scenario the Author still needs to write the non-secrets record to their wallet). It looks like this update moves all of this functionality to the Endorser? (Unless I'm not reading the code properly?) Let me know once you've updated the integration test, it's broken now so difficult to review ...

Hi @ianco,
I have fixed the integration test.
Also updated the unit tests.

I checked and tested the code, the author and endorser, both can invoke the endpoint to write the transaction to the ledger. However, the transaction is written to the ledger from the author's agent and so the records are being stored in author's wallet.
Can you point me, if I am missing something?


from ...handlers import transaction_acknowledgement_handler as test_module
from ...messages.transaction_acknowledgement import TransactionAcknowledgement
from ......connections.models.conn_record import ConnRecord
Copy link
Contributor

@sklump sklump May 10, 2021

Choose a reason for hiding this comment

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

There is an order to follow, for future reference:

  • import native package[s]
  • one blank line
  • from native package[s] import item[s]
  • one blank line
  • import third party package[s]
  • one blank line
  • from third party package[s] import item[s]
  • one blank line
  • from relative package[s] import item[s]: farthest to closest, one blank line between levels
  • alphabetical order within each grouping above.

So "from ......connections.models.conn_record ..." belongs at line 6.

  • from farthest to closest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @sklump,
Thanks for the feedback, I have fixed the order of import in the recent commit - 17c0548

import logging
import uuid
from asyncio import shield
Copy link
Contributor

Choose a reason for hiding this comment

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

So this would be:

import json
import logging
import uuid

from asyncio import shield
from time import time

from ....connections.models.conn_record import ConnRecord
from ....core.error import BaseError
from ....core.profile import ProfileSession
from ....indy.issuer import IndyIssuerError
from ....ledger.base import BaseLedger
from ....ledger.error import LedgerError
from ....messaging.credential_definitions.util import CRED_DEF_SENT_RECORD_TYPE
from ....messaging.schemas.util import SCHEMA_SENT_RECORD_TYPE
from ....storage.base import StorageRecord
from ....storage.error import StorageNotFoundError
from ....transport.inbound.receipt import MessageReceipt

from .messages.cancel_transaction import CancelTransaction
from .messages.endorsed_transaction_response import EndorsedTransactionResponse
from .messages.refused_transaction_response import RefusedTransactionResponse
from .messages.transaction_acknowledgement import TransactionAcknowledgement
from .messages.transaction_job_to_send import TransactionJobToSend
from .messages.transaction_request import TransactionRequest
from .messages.transaction_resend import TransactionResend
from .models.transaction_record import TransactionRecord
from .transaction_jobs import TransactionJob

It would be around this many imports that adding another one starts to risk duplication, if they're out of order.

@@ -1,4 +1,5 @@
import uuid
import json
Copy link
Contributor

Choose a reason for hiding this comment

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

flip them

@@ -16,6 +17,15 @@
from ..transaction_jobs import TransactionJob

from ..messages.transaction_request import TransactionRequest
from ..messages.transaction_acknowledgement import TransactionAcknowledgement
from ..transaction_jobs import TransactionJob
from .....ledger.base import BaseLedger
Copy link
Contributor

@sklump sklump May 10, 2021

Choose a reason for hiding this comment

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

... see? New code duplicates the existing TransactionJob import. Imports need sorting.

Copy link
Contributor

@sklump sklump left a comment

Choose a reason for hiding this comment

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

apart from gory imports, this looks good

HarshMultani-AyanWorks and others added 2 commits May 11, 2021 00:05
Taking latest changes done till EOD - 10/05/21
Signed-off-by: Harsh Multani <[email protected]>
@sklump
Copy link
Contributor

sklump commented May 11, 2021

Could you merge this with main? Then I'll merge it.

It might take some manual edits - main has been in significant churn.

@andrewwhitehead andrewwhitehead merged commit 6292b59 into openwallet-foundation:main May 11, 2021
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