handle consensus #350

andrewkmin · 2024-09-04T16:11:04Z

Summary & Motivation

$title

TLDR: Previously, the ACTIVITY_STATUS_CONSENSUS_NEEDED case was blatantly unhandled, resulting in misleading error messages/failure modes.

This change does the following:

makes changes to the base sdk-browser and sdk-server clients to handle non-terminal activity statuses
update various client libraries that depend on sdk-browser and sdk-server to allow for awaiting consensus
adds helpers to retrieve signed transactions + signed messages once consensus has been met on the Turnkey side (i.e. in case of a timeout)
@turnkey/viem now returns viem BaseErrors that wrap around TurnkeyActivityErrors. This also implicitly addresses err.walk is not a function #121
includes new example scripts for the ethers, viem, and solana packages (e.g. retry.ts and consensus.ts) which exercise this new functionality

The end user experience is such that if there is a policy enforcing consensus, you can configure a poller to await consensus to be met. See screen recordings in the testing section below.

TODO:

add unit tests (ideally — this requires policies)

How I Tested These Changes

local examples

Live polling for consensus during Ethers signing scripts:

Screen.Recording.2024-09-07.at.1.49.25.AM.mov

Consensus is not met at first within the polling retry limit, then later met, and the signature is retroactively retrieved + added to a Solana transaction
https://github.com/user-attachments/assets/af7fb6e3-46df-4785-a8ec-e2e7d4d1ff7e

Did you add a changeset?

If updating one of our packages, you'll likely need to add a changeset to your PR. To do so, run pnpm changeset. pnpm changeset will generate a file where you should write a human friendly message about the changes. Note how this (example) includes the package name (should be auto added by the command) along with the type of semver change (major.minor.patch) (which you should set).

These changes will be used at release time to determine what packages to publish and how to bump their version. For more context see this comment.

codesandbox-ci · 2024-09-04T16:12:24Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

zkharit · 2024-09-05T21:19:55Z

packages/ethers/src/index.ts

      });

-      return assertNonNull(activity?.signedTransaction);
+      if (activity.status !== "ACTIVITY_STATUS_COMPLETED") {


What exactly does this look like in the case where an activity requires consensus? Should an error be thrown?

Great question — ended up having the default behavior be to poll for 3 seconds, then return an error. At that point, it's up to the end-user to figure out how they want to handle the error. I've included some examples of handling in the various retry.ts files

examples/with-ethers/src/consensus.ts

r-n-o · 2024-09-10T14:55:38Z

examples/with-ethers/src/consensus.ts

+  // 1. Sign a raw payload (`eth_sign` style)
+  const message = "Hello Turnkey";
+  const signature = await connectedSigner.signMessage(message);
+  const recoveredAddress = ethers.verifyMessage(message, signature);


What makes this example "consensus.ts"? I think consensus vs. not will be driven by the organization setting? (1 vs N root quorum users?)

(basically, it doesn't seem necessary to have separate consensus examples unless I'm missing something 🤔 )

good call! not that useful. retry.ts should be enough to serve the purpose of demonstrating the scenario where one might be waiting for consensus

r-n-o · 2024-09-10T15:00:57Z

packages/ethers/src/index.ts

+        throw new TurnkeyActivityError({
+          message: `Unexpected activity status: ${activity.status}`,
+          activityId: activity.id,
+          activityStatus: activity.status as TActivityStatus,
+        });
+      }


I think the key is to throw a different error when the activity is in need of consensus. Something like:

if (activity.status == "ACTIVITY_STATUS_NEEDS_CONSENSUS") { throw new TurnkeyActivityNeedsConsensus({ message: `Activity requires consensus`, activityId: activity.id, activityStatus: activity.status as TActivityStatus, }); }

Then I agree with @andrewkmin, the retry or error behavior can be up to the caller.

ended up adding some bespoke errors to viem. the thing is it needs .walk() so I ended up extending the BaseError class. let me know what you think — if it adds unnecessary complexity, happy to cut it or reduce it down!

zkharit

💯 🚢 🚀

…d of null

r-n-o · 2024-09-13T17:10:36Z

examples/with-ethers/src/index.ts

+    // The following config is useful in contexts where an activity requires consensus.
+    // By default, if the activity is not initially successful, it will poll a maximum
+    // of 3 times with an interval of 10000 milliseconds.
+    //
+    // -----
+    //
+    // activityPoller: {
+    //   intervalMs: 10_000,
+    //   numRetries: 5,
+    // },


Is this still relevant?

EDIT: I think it is! Ignore this.

Yep! a bit ambiguous with the numbers though; will change them such that it might be a bit less confusing

r-n-o · 2024-09-13T17:14:32Z

packages/ethers/src/index.ts


      result = assertNonNull(activity?.result?.signRawPayloadResult);
    } else {
-      result = await this.client.signRawPayload({
+      // const trying = this.client as TurnkeyClient;


this comment was probably not meant to be a permanent comment? :)

r-n-o · 2024-09-13T17:17:47Z

packages/http/src/shared.ts

+  }
+}
+
+export function checkActivityStatus(input: {


checkActivityStatus implies it's a stateless innocuous function but I think this is more heavy than it looks: it will throw errors! So maybe assertActivityCompleted since that's kind of what it boils down to?

def more accurate!

r-n-o · 2024-09-13T17:19:19Z

packages/http/src/shared.ts

-  activityStatus: TActivityStatus | null;
-  activityType: TActivityType | null;
-  cause: Error | null;
+  activityId: TActivityId | undefined;


Why the change from null to undefined?

I went into this because of type compatibility, and ended up going with undefined for consistency. (functionally, null == undefined = true). Specifically, I feel like param: <Type X> | undefined better conveys the idea that the absence of the param is okay

Is activityId: TActivityId | undefined; equivalent to activityId?: TActivityId; then?

If that's the case I'm a definite +1 on the change; simpler to reason about!

r-n-o · 2024-09-13T17:20:34Z

packages/http/src/shared.ts

+    ].includes(activity.type)
+  ) {
+    throw new TurnkeyActivityError({
+      message: `Unexpected activity type: ${activity.type}`,


the message could be "Cannot get signature from activity type ...etc...". Otherwise it's a bit unclear why it's "unexpected".

r-n-o · 2024-09-13T17:22:07Z

packages/http/src/shared.ts

+      "ACTIVITY_TYPE_SIGN_RAW_PAYLOAD_V2",
+    ].includes(activity.type)
+  ) {
+    throw new TurnkeyActivityError({


I'm also thinking this should not be called "TurnkeyActivityError". It's likely an SDK-level / user-level issue if this function is called on the wrong activity type. We might need a plain BadArgumentError? Otherwise users will be led to believe the issue is within the turnkey activity processing when they get this error.

packages/http/src/shared.ts

r-n-o · 2024-09-13T17:25:08Z

packages/sdk-browser/scripts/codegen.js

+    let attempts = 0;
+
+    const pollStatus = async (activityId: string): Promise<TResponseType> => {
+      const pollBody = { activityId };
+      const pollData = await this.getActivity(pollBody) as ActivityResponse;
+
+      if (attempts > maxRetries) {
+        return handleResponse(pollData);
+      }
+
+      attempts += 1;


Nice. Finally fixing the "retry forever" behavior! 🏆

packages/sdk-browser/src/__types__/base.ts

r-n-o · 2024-09-13T17:27:57Z

packages/viem/src/index.ts

+export type TTurnkeyConsensusNeededErrorType = TurnkeyConsensusNeededError & {
+  name: "TurnkeyConsensusNeededError";
+};
+export class TurnkeyConsensusNeededError extends BaseError {


Instead of defining these errors here, can we pull that into http? Is there a way to comply with viem's error interface (implement walk is one thing; maybe others?) without importing viem's BaseError directly?

We'd need to implement this interface in our http/shared errors: https://github.com/wevm/viem/blob/408948bb74685e9d562cd39f50768f9b282463fe/src/errors/base.ts#L37-L83

I don't think it'd hurt anything to have our errors implement more methods than they need to, but maybe not something we need right now; feels a bit awkward to be pushed by a particular framework to implement these methods for all Turnkey errors.

+1 on the sentiment. I'm down to keep these encapsulated to just Viem for now. feels a bit awkward either way but we implement more at the base layer down the line if more cases arise!

r-n-o

🚀

r-n-o · 2024-09-13T22:53:19Z

packages/http/src/shared.ts

+  }
+}
+
+export function assertActivityCompleted(input: {


Now that I'm seeing this with fresh eyes: the activity ID isn't used anywhere really, we're simply comparing the passed in status to CONSENSUS_NEEDED and COMPLETED. Don't think it's necessarily worth changing this but...a bit of an odd interface given nothing guarantees the passed in status come from the passed in activity ID :/

Maybe it'd be clearer if a TActivity was expected and passed in instead?

Attempting this here: aa16d4d

The drawback of this approach is that it's a bit duplicative -- e.g. we return something like (paraphrasing)

{
...
result: activityData["result"]["activityResult"],
activity: activityData
...
}

So you see the overlap there, but having the result up front and center just makes access easier. The reason we needed this in the first place was originally for type-compatibility, but I realized it generally could be helpful to return all activity details

yeah it's definitely not super clear-cut, but I like this new version: assertActivityCompleted(activity); reads suuuper nice 😍

andrewkmin requested review from taylorjdawson, omkarshanbhag, moe-dev and zkharit September 4, 2024 16:28

zkharit reviewed Sep 5, 2024

View reviewed changes

andrewkmin force-pushed the andrew/consensus branch from 6ce58a0 to f7fcad9 Compare September 9, 2024 22:31

r-n-o reviewed Sep 10, 2024

View reviewed changes

zkharit approved these changes Sep 10, 2024

View reviewed changes

andrewkmin force-pushed the andrew/consensus branch 4 times, most recently from 8ffd1fb to c4952bd Compare September 11, 2024 00:20

andrewkmin requested a review from r-n-o September 11, 2024 00:30

andrewkmin force-pushed the andrew/consensus branch 2 times, most recently from b31ce3d to 7b99033 Compare September 11, 2024 19:32

andrewkmin added 12 commits September 12, 2024 01:18

handle consensus

bd39203

revised approach

0b6c9a3

wip: client lib support

eabeed2

add helpers for getting signatures

e9e0a7a

add activity poller config

c4efb29

add terminal activity statuses

6cf386e

prettify

2796eba

update client libraries again

3f1f6c7

update ethers

50f1723

misc cleanup

27bce6b

updated examples and added changeset

c988ed0

slight tweak to solana

91fbb29

andrewkmin force-pushed the andrew/consensus branch from 7b99033 to 854c180 Compare September 12, 2024 05:19

andrewkmin added 12 commits September 12, 2024 08:41

improved error handling

8f4336c

wip: cleanup types

f216f3d

complete viem error handling

b4b2983

delete consensus scripts

c6e861a

revise changesets

848f8d3

feedback: move shared utility functions to http

9ef0c3a

feedback: updated ethers

65781ae

move TERMINAL_ACTIVITY_STATUSES to http package; use undefined instea…

853ff85

…d of null

feedback: update viem

b039843

feedback: remove retry scripts

989e946

feedback: update solana example

1975153

update example READMEs

c65c409

andrewkmin force-pushed the andrew/consensus branch from 8e65169 to c65c409 Compare September 12, 2024 12:46

andrewkmin added 2 commits September 12, 2024 10:34

minor solana cleanup

0c0f79e

update all changesets

bdded80

andrewkmin force-pushed the andrew/consensus branch 3 times, most recently from 9029f26 to ccc9a93 Compare September 13, 2024 14:37

feedback: share signature, signatures, and signed transaction getters

375518f

andrewkmin force-pushed the andrew/consensus branch from ccc9a93 to 375518f Compare September 13, 2024 14:45

r-n-o reviewed Sep 13, 2024

View reviewed changes

feedback: update errors and changelogs

9bc9b98

andrewkmin force-pushed the andrew/consensus branch from cdf1bbd to 9bc9b98 Compare September 13, 2024 20:59

r-n-o approved these changes Sep 13, 2024

View reviewed changes

feedback: return full Activity in response

aa16d4d

r-n-o approved these changes Sep 16, 2024

View reviewed changes

andrewkmin merged commit 1e695f8 into main Sep 17, 2024
5 checks passed

andrewkmin deleted the andrew/consensus branch September 17, 2024 01:33

andrewkmin mentioned this pull request Sep 19, 2024

release consensus functionality #370

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

handle consensus #350

handle consensus #350

andrewkmin commented Sep 4, 2024 •

edited

Loading

codesandbox-ci bot commented Sep 4, 2024 •

edited

Loading

zkharit Sep 5, 2024

andrewkmin Sep 7, 2024

r-n-o Sep 10, 2024

andrewkmin Sep 11, 2024

r-n-o Sep 10, 2024

andrewkmin Sep 11, 2024

zkharit left a comment

r-n-o Sep 13, 2024 •

edited

Loading

andrewkmin Sep 13, 2024

r-n-o Sep 13, 2024

r-n-o Sep 13, 2024

andrewkmin Sep 13, 2024 •

edited

Loading

r-n-o Sep 13, 2024

andrewkmin Sep 13, 2024

r-n-o Sep 13, 2024

r-n-o Sep 13, 2024

r-n-o Sep 13, 2024

r-n-o Sep 13, 2024

r-n-o Sep 13, 2024

r-n-o Sep 13, 2024

andrewkmin Sep 13, 2024 •

edited

Loading

r-n-o left a comment

r-n-o Sep 13, 2024

andrewkmin Sep 16, 2024

r-n-o Sep 16, 2024

handle consensus #350

handle consensus #350

Conversation

andrewkmin commented Sep 4, 2024 • edited Loading

Summary & Motivation

How I Tested These Changes

Did you add a changeset?

codesandbox-ci bot commented Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zkharit left a comment

Choose a reason for hiding this comment

r-n-o Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewkmin Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewkmin Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

r-n-o left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewkmin commented Sep 4, 2024 •

edited

Loading

codesandbox-ci bot commented Sep 4, 2024 •

edited

Loading

r-n-o Sep 13, 2024 •

edited

Loading

andrewkmin Sep 13, 2024 •

edited

Loading

andrewkmin Sep 13, 2024 •

edited

Loading