-
Notifications
You must be signed in to change notification settings - Fork 15
Update for v.2025.12.2 #66
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
Conversation
c12e17a to
c9324b2
Compare
2019de0 to
7fb964d
Compare
r-n-o
left a comment
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.
Consider me impressed!
- the builder arg for app proof generation is looking great: it's out of the way for most callers, and easy to use for interested parties
- the wrapper struct is adding a bit of extra boilerplate (
.result), but I think that'll be useful for more than just app proofs! Having the activity ID returned is nice for other reasons (e.g. get votes, or get full activity by ID to get the intent, that kind of thing)
| #[derive(Debug, Clone)] | ||
| pub struct ActivityResult<T> { | ||
| /// The typed result specific to this activity (e.g., `CreateWalletResult`, `SignTransactionResult`) | ||
| pub result: T, | ||
|
|
||
| /// The unique identifier for this activity | ||
| pub activity_id: String, | ||
|
|
||
| /// The status of the activity | ||
| pub status: ActivityStatus, | ||
|
|
||
| /// Proofs that can be used to verify the activity was performed in a secure Turnkey enclave. | ||
| pub app_proofs: Vec<AppProof>, | ||
| } |
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.
dope! 🥇
| /// Enable app proof generation for all activity requests. | ||
| /// | ||
| /// When enabled, the server will return app proofs with each response | ||
| /// that can be independently verified. | ||
| /// | ||
| /// # Example | ||
| /// ```rust,ignore | ||
| /// let client = TurnkeyClient::builder() | ||
| /// .api_key(api_key) | ||
| /// .build()? | ||
| /// .with_app_proofs(); | ||
| /// ``` | ||
| pub fn with_app_proofs(mut self) -> Self { | ||
| self.generate_app_proofs = Some(true); | ||
| self | ||
| } |
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.
Beautiful!
| /// | ||
| /// {description} | ||
| pub async fn {fn_name}(&self, organization_id: String, timestamp_ms: u128, params: immutable_activity::{activity_intent}) -> Result<immutable_activity::{activity_result}, TurnkeyClientError> {{ | ||
| pub async fn {fn_name}(&self, organization_id: String, timestamp_ms: u128, params: immutable_activity::{activity_intent}) -> Result<ActivityResult<immutable_activity::{activity_result}>, TurnkeyClientError> {{ |
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.
Arguably this interface change is a breaking change. So maybe we should mark it as such in the changelog? Users of the SDK will have to update their code.
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.
You're very right. Fixed
client/CHANGELOG.md
Outdated
| ### Other | ||
|
|
||
| - Define activity result wrapper so that we return app proofs | ||
| - add with_app_proofs() method to client to toggle generate_app_proofs so that all calls don't have to be updated to populate the new optional field |
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.
sounds like this should be in the ### Added section rather than "other"! Pretty big addition!
(and see my other comment, I think it's worth flagging the new return type: we go from typed results to a typed wrapper struct)
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 misunderstood the changelog's structure haha I pattern-matched the entry right below. Fixed!
BREAKING CHANGE: consumers must now handle wrapped activity results
7fb964d to
7d9ddc3
Compare
r-n-o
left a comment
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.
🥇
generate_app_proofsby adding awith_app_proofs()method toclient