-
Notifications
You must be signed in to change notification settings - Fork 97
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: enhanced delegation enforcement checks #761
base: main
Are you sure you want to change the base?
Conversation
adaf5d6
to
3a89fc0
Compare
size-limit report 📦
|
* chore: limit npm version to 9 in ci for compatibility with node 16 * changelog
…ations during initialization
…nity/agent-js into kyle/SDK-1193-delegation-enforcement
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.
for (it in collection) {
do something with it
}
is easier to understand than
let current = something else;
for (it in collection) {
do something with current
current = it
}
now check current for something // last value in collection
Co-authored-by: Eric Swanson <[email protected]>
Co-authored-by: Eric Swanson <[email protected]>
Co-authored-by: Eric Swanson <[email protected]>
Co-authored-by: Eric Swanson <[email protected]>
Valid point! I've refactored to use a |
} | ||
|
||
// Ensure that public keys are not repeated in the chain. | ||
options.previous.delegations.reduce((previous, current) => { |
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.
Given that options.previous
is a DelegationChain, is it really necessary to check that options.previous.delegations
has no repeated delegations?
Asked another way, does DelegationChain
have an invariant that delegations
has no repeated public keys?
If so, it should be sufficient (and simpler) here to check that to
is not found among options.previous.delegations
.
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.
If TypeScript was the runtime language of the web, we could be confident that the other chain is a valid DelegationChain. However, if we want runtime validation, we have to do the checks ourself
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.
good thing MAXIMUM_DELEGATION_CHAIN_LENGTH is only 20. Pretty sure this is O(n^2) or possibly O(n^2 log n).
Description
So far, the JavaScript agent has not enforced checks around delegation constraints. This meant that developers had to encounter errors later in the development process, often with less helpful certificate errors.
These changes introduce the following constraints, which now throw errors during the creation of a delegation chain:
This complies with the constraints indicated in the Internet Computer Interface Specification
Fixes # (issue)
How Has This Been Tested?
New and updated unit tests
Checklist: