dfinity / agent-js

A collection of libraries and tools for building software around the Internet Computer, in JavaScript.
https://agent-js.icp.xyz
Apache License 2.0
151 stars 94 forks source link

feat: add support for proof of absence in certificate lookups #878

Closed nathanosdev closed 3 months ago

nathanosdev commented 4 months ago

Description

Adds support for proof of absence in certificate lookups. Previously a lookup with either return a value or undefined if the value cannot be found. Now the result of a lookup will differentiate between Found, Unknown and Absent, which allows clients to tell the difference between a value that is provably absent from the tree, or one that might have been pruned from the tree (maliciously or otherwise).

Fixes # SDK-1219

How Has This Been Tested?

I've added new unit tests to cover the new lookup functionality.

Checklist:

nathanosdev commented 4 months ago

@krpeacock it would be good to get your opinion on the interface here. I've kept it so that Certificate.lookup() has the same behaviour as before. To get the new lookup behaviour, clients would need to import the lookup_path() function.

Would you prefer to keep it like this or to propagate the new behaviour to the Certificate.lookup() function too?

It's also possible now to make the agent more robust by retrying requests when requested certificate entries are Unknown since that would imply the replica is acting maliciously. Retrying these requests would hit another (hopefully honest) replica and get a certificate that has the expected values. Examples are when we do lookups for time or request_status, but I think that would be best done in a follow up task.

krpeacock commented 4 months ago

@nathanosdev I think that extending the changes to the lookup and also exporting lookup_path is a fine path forward.

I'd rather not also introduce the retry logic for unknown at this stage. It makes me want to think about a different design for retry logic, perhaps offering a more fine-grained set of options giving devs more control over what circumstances the agent will retry under

nathanosdev commented 4 months ago

@nathanosdev I think that extending the changes to the lookup and also exporting lookup_path is a fine path forward.

I'd rather not also introduce the retry logic for unknown at this stage. It makes me want to think about a different design for retry logic, perhaps offering a more fine-grained set of options giving devs more control over what circumstances the agent will retry under

Sounds good, I'll extend the changes to lookup.

nathanosdev commented 4 months ago

@krpeacock I've extended the changes to the lookup method now and used the lookupResultToBuffer everywhere that references the lookup method to keep the same behaviour as before for internal calls to lookup.

There seems to be something up with the Check PR title job. There was a new breaking version of conventional-changelog-angular a few days ago so I assumed that was the problem, but after locking it down to the previous version that was published several months ago it's still broken. Do you have any idea what might be going on?

krpeacock commented 4 months ago

There seems to be something up with the Check PR title job.

Oh, you just have to remove the exclamation mark from the pr title, even though it's proper semantics for conventional commits. Leave it in the change log though

nathanosdev commented 4 months ago

It doesn't seem happy with it still, the error message is Error: Must use import to load ES Module: /action/node_modules/conventional-changelog-angular/src/index.js