Lido offchain plugins p1 #435

luketchang · 2023-09-05T03:27:04Z

Motivation

Start offchain work needed to support Lido. OpRequestBuilder.build() needed to be async due to plugins like Uniswap needing to make network calls for fetching routes.

Solution

Split op request builder plugins out of core package (e.g., don't want to import uniswap sdk into core)
Refactor builder to collect array of plugin fn promises that resolve to unwraps, actions, refunds, and metadatas to enqueue
Add wsteth adapter plugin which supports sending ETH to wsteth in return for wsteth
NOTE: we still don't have plugins for swapping out of wsteth back to weth (coming in next PR with Uniswap plugin)

Proof

https://www.loom.com/share/6e5d5094613247788783baedf0e16b63?sid=b4ade8b1-a86c-4613-99f0-f38bad62b579

PR Checklist

added tests
updated documentation
added changeset if necessary
tested in dev/testnet
tested site with snap (we haven't automated this yet)
re-built & tested circuits if any of them changed
updated contracts storage layout (if contracts were updated)

changeset-bot · 2023-09-05T03:27:09Z

🦋 Changeset detected

Latest commit: 7e96332

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 15 packages

Name	Type
@nocturne-xyz/core	Major
@nocturne-xyz/op-request-plugins	Patch
@nocturne-xyz/frontend-sdk	Patch
@nocturne-xyz/e2e-tests	Patch
@nocturne-xyz/deploy	Patch
@nocturne-xyz/idb-kv-store	Patch
@nocturne-xyz/local-prover	Patch
@nocturne-xyz/persistent-log	Patch
@nocturne-xyz/bundler	Patch
@nocturne-xyz/deposit-screener	Patch
@nocturne-xyz/insertion-writer	Patch
@nocturne-xyz/subtree-updater	Patch
@nocturne-xyz/test-actor	Patch
@nocturne-xyz/snap	Patch
@nocturne-xyz/offchain-utils	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

luketchang · 2023-09-05T03:27:16Z

Current dependencies on/for this PR:

main
- PR Lido offchain plugins p1 #435 👈
  - PR Lido offchain plugin work p2 #436

This comment was auto-generated by Graphite.

Sladuca · 2023-09-05T21:38:21Z

packages/core/src/operationRequest/builder.ts

+
+  // add a plugin promise to await, resolves to unwraps, refunds, and actions to enqueue
+  // returns `this` so it's chainable
+  pluginFn(pluginPromise: Promise<PluginFnResult>): this;


we can probably have a better name than pluginFn - instead, how about enqueueAsyncPlugin or something like that?

well the its not the plugin itself that's being called, a function on the plugin (plugins will have multiple methods). Think this is probably fine. Fn or call but same thing

Sladuca · 2023-09-05T21:51:13Z

packages/core/src/operationRequest/operationRequest.ts

@@ -5,10 +5,13 @@ import { Action, Address, Asset, OperationMetadata } from "../primitives";
 const ONE_DAY_SECONDS = 24 * 60 * 60;

 // A joinsplit request is an unwrapRequest plus an optional payment
-export interface JoinSplitRequest {
+export interface JoinSplitRequest extends UnwrapRequest {


Kinda nit, but this type is effectively saying a JoinSplitRequest is a special case of UnwrapRequest, when we really want the opposite semantics - UnwrapRequest is a special case of JoinSplitRequest.

We can get the latter semantics via the following:

export interface JoinSplitRequest { asset: Asset; unwrapValue: unwrapValue; payment?: ConfidentialPayment; } export type UnwrapRequest = Omit<JoinSplitRequest, "payment">;

Here, a JoinSplitRequest is not assignable to a variable of type UnwrapRequest, but an UnwrapRequest is assignable to a variable of type JoinSplitRequest

Sladuca · 2023-09-05T21:52:36Z

packages/op-request-plugins/CHANGELOG.md

+
+### Unreleased
+
+- Package name migrated from `sdk` to `core`


clear changelog entries from core/sdk

Sladuca · 2023-09-05T21:56:41Z

packages/op-request-plugins/src/WstethAdapterPlugin.ts

+    ...inner,
+    use: use,
+    convertWethToWsteth(amount: bigint) {
+      const prom = new Promise<PluginFnResult>((resolve) => {


technically don't need to enqueue a promise here, as there's no async.

Well its for type consistency since we want all plugins to add to same array and have same interface to live by (enqueue actions, unwraps, refunds for op request builder to handle at .build()). Other option is to just push directly to array but would rather keep devX consistent across plugins

this is not what the ERC20 plugin is doing - in this case we should force it to enqueue a promise as well.

Wait but it is enqueuing promise, its same pattern as wsteth adapter plugin
https://github.com/nocturne-xyz/monorepo/pull/435/files#diff-4adf8b51946131cc888242ac38ca0ada71088ac788898b52540479972ab48382R84

oh oops I see.

should we also make the "base" methods all enqueue promises as well then? Someone could do something like:

await builder .use(Erc20Plugin) .transfer(...) // async .unwrap(...) // sync .build()

in .build(), the unwrap() will be applied before, not after the transfer

Yeah I can do this for consistency

Sladuca · 2023-09-05T22:01:33Z

packages/op-request-plugins/src/WstethAdapterPlugin.ts

+    use: use,
+    convertWethToWsteth(amount: bigint) {
+      const prom = new Promise<PluginFnResult>((resolve) => {
+        const chainId = this._op.chainId;


we might need to be careful how this gets bound in the closure. It could very well be that if you call use after calling convertWethToWsteth here, the this inside the closure could be pointing to the pre-extension builder

not sure I'm understanding, what might happen here?

suppose user does something like:

builder .use(Erc20Plugin) .transfer(...) .use(SomeOtherPlugin) .someOtherPluginMethod(...);

when .transfer(...) is called, it'll create a closure over a context where this points to the builder object returned by .use(Erc20Plugin) and pass it into the promise constructor. Then the second call to .use(SomeOtherPlugin) is gonna return a new object with all of the methods / properties copied over, but the context closed over by the closure created during transfer(...) will still have this pointing to the object returned by .use(Erc20Plugin).

This isn't really an issue to fix as much as it is something I wanted to flag that I noticed just now - this might be a problem with the current synchronous implementation (honestly not sure how "object methods" get their this bound when they're copied to another object), but it's definitely a problem here depending on whether shallow or deep copies are being made (and it's not immediately clear to me which is the case, it may actually depend on the properties / plugins involved)

… req

…in op req builder is consistent with actual

…e at build() step

luketchang requested review from Sladuca and panieldark as code owners September 5, 2023 03:27

luketchang marked this pull request as draft September 5, 2023 03:27

luketchang changed the title ~~WIP: lido offchain plugins~~ WIP: lido offchain plugins p1 Sep 5, 2023

luketchang changed the title ~~WIP: lido offchain plugins p1~~ Lido offchain plugins p1 Sep 5, 2023

luketchang marked this pull request as ready for review September 5, 2023 14:32

luketchang mentioned this pull request Sep 5, 2023

Lido offchain plugin work p2 #436

Merged

7 tasks

Sladuca reviewed Sep 5, 2023

View reviewed changes

luketchang force-pushed the luke/lido-offchain branch 2 times, most recently from 090e0e9 to 7448898 Compare September 7, 2023 03:21

luketchang added 15 commits September 6, 2023 23:52

save

61ee5f2

add wsteth adapter plugin and unit test

cf2791e

export balancer and uniswap ifaces from contracts

f3c3fcb

save op-request-plugins new packages, snap failing build

11c47c7

monorepo builds again after removing __test export from core

d5dec6f

op request builder collects plugin fn proms

35b028e

plugins tests pass again with async builder build()

889eff1

nit: fix ordering in erc20 plugin fn

7926ea7

changesets

809b1e0

fix changeset

45ccb5f

addr comments, clear plugins changelog, change inheritance for unwrap…

2205b90

… req

rebase

9615b13

prog: separate enqueuing and processing of items so order of actions …

fdd0359

…in op req builder is consistent with actual

fix redis build issues

06b6cf9

refactor builder, remove joinsplits by asset and metadata and only us…

7c69b6f

…e at build() step

luketchang force-pushed the luke/lido-offchain branch from 7448898 to 7c69b6f Compare September 7, 2023 03:56

Sladuca approved these changes Sep 7, 2023

View reviewed changes

fix e2e test build after insertion writer e2e test uses core plugin

7e96332

luketchang enabled auto-merge (squash) September 7, 2023 15:49

luketchang merged commit 6abd69b into main Sep 7, 2023
3 checks passed

luketchang deleted the luke/lido-offchain branch September 7, 2023 16:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lido offchain plugins p1 #435

Lido offchain plugins p1 #435

luketchang commented Sep 5, 2023 •

edited

Loading

changeset-bot bot commented Sep 5, 2023 •

edited

Loading

luketchang commented Sep 5, 2023 •

edited

Loading

Sladuca Sep 5, 2023

luketchang Sep 6, 2023

Sladuca Sep 5, 2023

Sladuca Sep 5, 2023

Sladuca Sep 5, 2023

luketchang Sep 6, 2023 •

edited

Loading

Sladuca Sep 6, 2023

luketchang Sep 6, 2023

Sladuca Sep 6, 2023

Sladuca Sep 6, 2023 •

edited

Loading

luketchang Sep 6, 2023

Sladuca Sep 5, 2023

luketchang Sep 6, 2023

Sladuca Sep 6, 2023 •

edited

Loading

Lido offchain plugins p1 #435

Lido offchain plugins p1 #435

Conversation

luketchang commented Sep 5, 2023 • edited Loading

Motivation

Solution

Proof

PR Checklist

changeset-bot bot commented Sep 5, 2023 • edited Loading

🦋 Changeset detected

luketchang commented Sep 5, 2023 • 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

luketchang Sep 6, 2023 • 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

Sladuca Sep 6, 2023 • 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

Sladuca Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

luketchang commented Sep 5, 2023 •

edited

Loading

changeset-bot bot commented Sep 5, 2023 •

edited

Loading

luketchang commented Sep 5, 2023 •

edited

Loading

luketchang Sep 6, 2023 •

edited

Loading

Sladuca Sep 6, 2023 •

edited

Loading

Sladuca Sep 6, 2023 •

edited

Loading