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

feat: useable and tested scratchpads #2671

Merged
merged 7 commits into from
Jan 29, 2025

Conversation

grumbach
Copy link
Member

@grumbach grumbach commented Jan 27, 2025

  • Now scratchpad have their own module clearly separated from Vault and Vaults use the generic Scratchpad API.
  • Exactly like Pointers, Scratchpads have a put/get/create/update API that allows for manual or user friendly use of the scratchpad.
  • E2E tests for Scratchpad to prove they work as intended. They could serve as usage examples as well.
  • GraphEntry, Pointer and Scratchpad use PaymentOption now, unifying the payment process

@grumbach grumbach enabled auto-merge January 27, 2025 05:36
@grumbach grumbach force-pushed the useable_scratchpads branch 2 times, most recently from f4c263a to 28e8c26 Compare January 27, 2025 06:13
ant-protocol/src/storage/scratchpad.rs Outdated Show resolved Hide resolved
ant-protocol/src/storage/scratchpad.rs Outdated Show resolved Hide resolved
/// Create a new Scratchpad without provding the secret key
/// It is the caller's responsibility to ensure the signature is valid (signs [`Scratchpad::bytes_for_signature`]) and the data is encrypted
/// It is recommended to use the [`Scratchpad::new`] method instead when possible
pub fn new_with_signature(
Copy link
Member

Choose a reason for hiding this comment

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

to make the public API cleaner, this function shall not be exposed ?
also, this function means input_data is not encrypted, which is un-consistent with new function and claims made in other places.
also, this one doesn't call the is_valid self check ?

I'd suggest:

  • not use this in the public API
  • make new function use this (if retained as a private helper)

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking of keeping this for folks that cannot provide us with the Secret key but can sign messages/encrypt. This is typically the case with hardware wallets, where one cannot get the secret key out, but can perfectly sign messages.

Copy link
Member

Choose a reason for hiding this comment

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

even we accept such input, the signature and size shall still be checked.
i.e. the verify function shall be called

ant-protocol/src/storage/scratchpad.rs Outdated Show resolved Hide resolved
autonomi/src/client/data_types/scratchpad.rs Outdated Show resolved Hide resolved
autonomi/src/client/data_types/scratchpad.rs Outdated Show resolved Hide resolved
ant-protocol/src/storage/scratchpad.rs Outdated Show resolved Hide resolved
autonomi/src/client/data_types/scratchpad.rs Outdated Show resolved Hide resolved
/// Create a new Scratchpad without provding the secret key
/// It is the caller's responsibility to ensure the signature is valid (signs [`Scratchpad::bytes_for_signature`]) and the data is encrypted
/// It is recommended to use the [`Scratchpad::new`] method instead when possible
pub fn new_with_signature(
Copy link
Member

Choose a reason for hiding this comment

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

even we accept such input, the signature and size shall still be checked.
i.e. the verify function shall be called

counter,
signature,
}
}
Copy link
Member

Choose a reason for hiding this comment

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

shall the self.verify() function still to be called ?

false
}
/// Verifies that the Scratchpad signature is valid
pub fn verify(&self) -> bool {
Copy link
Member

@maqi maqi Jan 28, 2025

Choose a reason for hiding this comment

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

shall the size to be checked as well?
same to the other data_types

Also, I notice there is scratchpad_verify which checks both signature and size for ScratchPad
I think normally, the verify indicates both, verify_signature and verify_size (or is_size_too_big) verifies individual part.
I'd suggest update the names/function calls of all data_types' verify to make them consistant.

@grumbach grumbach added this pull request to the merge queue Jan 28, 2025
@maqi maqi removed this pull request from the merge queue due to a manual request Jan 28, 2025
@maqi maqi added this pull request to the merge queue Jan 28, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 28, 2025
@maqi maqi added this pull request to the merge queue Jan 28, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 28, 2025
@maqi maqi force-pushed the useable_scratchpads branch from 317b98b to ab8327d Compare January 28, 2025 14:57
@maqi maqi enabled auto-merge January 28, 2025 14:58
@maqi maqi added this pull request to the merge queue Jan 28, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 28, 2025
@maqi maqi added this pull request to the merge queue Jan 28, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 28, 2025
@grumbach grumbach added this pull request to the merge queue Jan 29, 2025
Merged via the queue into maidsafe:main with commit 937b4d4 Jan 29, 2025
25 checks passed
@grumbach grumbach deleted the useable_scratchpads branch January 29, 2025 01:26
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.

3 participants