fix: multiuse invites with did peer 4 #3112
Conversation
This corrects an issue where did:peer:4 connection records were failing to be resolved on inbound message, resulting in the multiuse invitation that created the connection being resolved instead. Fixes hyperledger#3111. Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
dbluhm
force-pushed
the
fix/did-peer-4-conn-lookup
branch
from
3404e69
to
bdc7ccf
Compare
|
OK to merge with the SonarCloud state? @dbluhm what do you think? |
|
I'm okay with it being merged as is but I am also happy to get a quick unit test written. Would not take a lot to cover the changes since it wasn't a lot of changes. |
|
Sometimes reformatted code will make sonarcloud report areas that didn't have any logic changes. I think that happened a bit here. Could think about a couple tests for the new code. |
Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
|
Sonar cloud is still unhappy but I'd say the new unit tests + integration tests will be adequate to cover these changes. |
|
@jamshale https://github.com/hyperledger/aries-cloudagent-python/actions/runs/10045120793/job/27761704780?pr=3112#step:4:18060 |
This test does have false negatives quite often, and it doesn't look very related. I think it's probably the case, as other tests would have failed as well. Let's see if it passes a new run. |
|
@dbluhm This test is actually failing locally. I think it must be related to the changes. I'm not sure why the |
|
Interesting -- I'll dig in! Thanks for checking locally |
Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
|
@dbluhm — sorry to bug you, but another failed test in the PR Tests. Looks to be in the same area as the most recent commit. |
Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
Yeah, pushed while my tests were running locally still 🙂 PR tests should be good now. I think int tests are too but they take a while to run locally even when narrowed to just the endorser scenarios. |
|
| @@ -772,20 +767,17 @@ async def set_transaction_my_job(self, record: ConnRecord, transaction_my_job: s | |||
| return tx_job_to_send | |||
|
|
|||
| async def set_transaction_their_job( | |||
| self, tx_job_received: TransactionJobToSend, receipt: MessageReceipt | |||
| self, tx_job_received: TransactionJobToSend, connection: ConnRecord | |||
There was a problem hiding this comment.
I was looking at this for a bit and didn't understand why it used MessageReceipt here?
There was a problem hiding this comment.
Indeed; my impression is that it was just a question of the implementer being unfamiliar with the request context or something along those lines.
This corrects an issue where did:peer:4 connection records were failing to be resolved on inbound message, resulting in the multiuse invitation that created the connection being resolved instead.
Fixes #3111.