Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: single-sig IPEX apply, offer, agree #234
feat: single-sig IPEX apply, offer, agree #234
Changes from 3 commits
00a3009
e27005a
9e218ad
5409fea
0db79b2
e6fb785
b54026b
7af8741
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 132 in examples/integration-scripts/credentials.test.ts
GitHub Actions / Build, lint, and test on Node 18 and macOS-latest
Check warning on line 132 in examples/integration-scripts/credentials.test.ts
GitHub Actions / Build, lint, and test on Node 20 and macOS-latest
Check warning on line 132 in examples/integration-scripts/credentials.test.ts
GitHub Actions / Build, lint, and test on Node 18 and ubuntu-latest
Check warning on line 132 in examples/integration-scripts/credentials.test.ts
GitHub Actions / Build, lint, and test on Node 20 and ubuntu-latest
Check warning on line 250 in examples/integration-scripts/credentials.test.ts
GitHub Actions / Build, lint, and test on Node 18 and macOS-latest
Check warning on line 250 in examples/integration-scripts/credentials.test.ts
GitHub Actions / Build, lint, and test on Node 20 and macOS-latest
Check warning on line 250 in examples/integration-scripts/credentials.test.ts
GitHub Actions / Build, lint, and test on Node 18 and ubuntu-latest
Check warning on line 250 in examples/integration-scripts/credentials.test.ts
GitHub Actions / Build, lint, and test on Node 20 and ubuntu-latest
Check warning on line 273 in examples/integration-scripts/credentials.test.ts
GitHub Actions / Build, lint, and test on Node 18 and macOS-latest
Check warning on line 273 in examples/integration-scripts/credentials.test.ts
GitHub Actions / Build, lint, and test on Node 20 and macOS-latest
Check warning on line 273 in examples/integration-scripts/credentials.test.ts
GitHub Actions / Build, lint, and test on Node 18 and ubuntu-latest
Check warning on line 273 in examples/integration-scripts/credentials.test.ts
GitHub Actions / Build, lint, and test on Node 20 and ubuntu-latest
Check warning on line 304 in examples/integration-scripts/credentials.test.ts
GitHub Actions / Build, lint, and test on Node 18 and macOS-latest
Check warning on line 304 in examples/integration-scripts/credentials.test.ts
GitHub Actions / Build, lint, and test on Node 20 and macOS-latest
Check warning on line 304 in examples/integration-scripts/credentials.test.ts
GitHub Actions / Build, lint, and test on Node 18 and ubuntu-latest
Check warning on line 304 in examples/integration-scripts/credentials.test.ts
GitHub Actions / Build, lint, and test on Node 20 and ubuntu-latest
Check warning on line 330 in examples/integration-scripts/credentials.test.ts
GitHub Actions / Build, lint, and test on Node 18 and macOS-latest
Check warning on line 330 in examples/integration-scripts/credentials.test.ts
GitHub Actions / Build, lint, and test on Node 18 and macOS-latest
Check warning on line 330 in examples/integration-scripts/credentials.test.ts
GitHub Actions / Build, lint, and test on Node 20 and macOS-latest
Check warning on line 330 in examples/integration-scripts/credentials.test.ts
GitHub Actions / Build, lint, and test on Node 20 and macOS-latest
Check warning on line 330 in examples/integration-scripts/credentials.test.ts
GitHub Actions / Build, lint, and test on Node 18 and ubuntu-latest
Check warning on line 330 in examples/integration-scripts/credentials.test.ts
GitHub Actions / Build, lint, and test on Node 18 and ubuntu-latest
Check warning on line 330 in examples/integration-scripts/credentials.test.ts
GitHub Actions / Build, lint, and test on Node 20 and ubuntu-latest
Check warning on line 330 in examples/integration-scripts/credentials.test.ts
GitHub Actions / Build, lint, and test on Node 20 and ubuntu-latest
Check warning on line 375 in examples/integration-scripts/credentials.test.ts
GitHub Actions / Build, lint, and test on Node 18 and macOS-latest
Check warning on line 375 in examples/integration-scripts/credentials.test.ts
GitHub Actions / Build, lint, and test on Node 20 and macOS-latest
Check warning on line 375 in examples/integration-scripts/credentials.test.ts
GitHub Actions / Build, lint, and test on Node 18 and ubuntu-latest
Check warning on line 375 in examples/integration-scripts/credentials.test.ts
GitHub Actions / Build, lint, and test on Node 20 and ubuntu-latest
Check warning on line 421 in examples/integration-scripts/credentials.test.ts
GitHub Actions / Build, lint, and test on Node 18 and macOS-latest
Check warning on line 421 in examples/integration-scripts/credentials.test.ts
GitHub Actions / Build, lint, and test on Node 20 and macOS-latest
Check warning on line 421 in examples/integration-scripts/credentials.test.ts
GitHub Actions / Build, lint, and test on Node 18 and ubuntu-latest
Check warning on line 421 in examples/integration-scripts/credentials.test.ts
GitHub Actions / Build, lint, and test on Node 20 and ubuntu-latest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure on when
i
should be part of the attributes here. It is for grant and not for apply. From what I remember when checking out keripy, it seems to auto addi
if an exn message is created there.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iFergal Do you have a link to the lines of code in keripy where this is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lenkan Sure - https://github.com/WebOfTrust/keripy/blob/development/src/keri/peer/exchanging.py#L333
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a look here and it is being done in here as well:
signify-ts/src/keri/app/exchanging.ts
Lines 194 to 214 in ac6dac4
I think this is more explicit, we can amend the interface for the payload parameter in
createExchangeMessage
to be something likethat way there is only one way to specify the recipient and we can remove the recipient parameter. This would be more in line with the changes I proposed in #222.
I don't mean to make that change here, but it could be done as a separate PR to tidy up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see, though that line of code is never being hit for IPEX since
recipient
is always passed asundefined
tocreateExchangeMessage
. The only other place thatcreateExchangeMessage
is called is from thesend
method of exchanges, which also doesn't pass the recipient.Maybe @pfeairheller can shed more light on when it should be passed as an attribute too (at least for IPEX).
But agree on the interface tidy up, makes sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lenkan We discussed this in the dev call today, and I also can see you have #252 open that also does this. I've pushed the change for the CICD and compose file only to see if it will pass now. Not sure if additionally all of the other changes you have in draft are required too or not with this new version.
cc @rodolfomiranda
Edit - OK, looks like the tests are indeed broken for the new version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iFergal The changes in
aiding.ts
is needed for the upgrade. The other changes were only to highlight what is broken.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lenkan Do you think that PR will merge this week or shall I cherry pick changes to
aiding.ts
across?edit - hitting this now, will try to dig into why when I've more time. (on first call to
await grantMultisig
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that is the exact same error I reproduced in #252. I created the issue in keria that needs resolution WebOfTrust/keria#230. I have not yet debugged keria for that issue.
It would be good to see if it can be reproduced in plain keripy first I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made some other adjustments in my latest push -
smids
/rmids
only changed on group creation, not rotation.There were also some changes in keripy on not using hard coded salts so our tests can't assume the agent AID anymore.
Once WebOfTrust/keria#243 merges, the only failing test is the one you described for vlei issuance. Haven't got a chance to support on that yet.