feat: update oval node to support oval factory deployments #83

md0x · 2024-07-15T19:25:34Z

Changes proposed in this PR:

Support for Oval Factories: Implement the new feature that allows permissionless deployments of Oval. Oval factories
New Configuration: Add a new config ovalConfigsShared
Introduce WalletManager to allocate wallets for the unlocks. WalletManager
Add OvalDiscovery service to find permissionless deployments of Oval throughout the factories. OvalDiscovery
No changes in how searchers interact with Oval. Searchers can use the oval-headers to specify a particular Oval for unlocks, or they can opt to not specify and rely on the Oval mechanism to find the necessary instance.

Signed-off-by: Pablo Maldonado <[email protected]>

linear · 2024-07-15T19:26:27Z

UMA-2703 update oval node to support oval factory deployments

Signed-off-by: Pablo Maldonado <[email protected]>

mrice32

This looks great! I think this is really well modularized. Only a few small comments

mrice32 · 2024-07-18T08:21:51Z

src/lib/walletManager.ts

+type WalletUsed = {
+    walletPubKey: string;
+    targetBlock: number;
+    count: number;


Does count mean we can use a wallet more than once in a single block? Interesting... I hadn't really considered what the node would do if it ran out of wallets. I think this makes sense.

Correct, this is to handle scenarios where we have more oval-instances than unlockers and spikes in the number of unlocks.

I will also add alerts in those cases.

mrice32 · 2024-07-18T08:34:05Z

test/walletManager.ts

@@ -0,0 +1,134 @@
+import { expect } from 'chai';


+1 on having unit tests for this module

mrice32 · 2024-07-18T08:39:56Z

src/lib/walletManager.ts

+    private ovalDiscovery: OvalDiscovery;
+    private wallets: Record<string, Wallet> = {};
+    private sharedWallets: Map<string, Wallet> = new Map();
+    private sharedWalletUsage: Map<string, Array<WalletUsed>> = new Map();


Why did you choose to have an Array to track wallet usage rather than a mapping?

For instance, I could imagine a mapping from block->OvalInstance->WalletUsed might be a decent alternative: simple to drop blocks once they've passed (just delete from the map) and it's easy to tell if there's already been a wallet assigned (no array traversal/find).

Thanks, that’s a better data structure from the record cleanup. However, initially, I proposed the data structure with this idea in mind:

We want to use the same unlocker for a given oval instance, regardless of the target block. This approach is also due to a cleanup running every 5 minutes, preventing old assignments from being reused, focusing only on recent assignments.

The reason for this is that multiple unlocks to the same oval instance in target blocks close to each other are likely part of the same backrun. They belong to the same price auction, meaning nonce collisions are not possible if we reuse the unlocker. Do you agree with this?

The previous data structure seemed to work fine with that assumption. However, I do like removing the array and using only mappings that we still need to traverse, for the reason I explained above.

I’ve changed the data structure in the latest commits.

Signed-off-by: Pablo Maldonado <[email protected]>

daywiss · 2024-07-22T15:40:30Z

src/lib/ovalDiscovery.ts

+        this.findOval(FACTORIES_GENESIS_BLOCK);
+    }
+
+    public async findOval(fromBlock: number) {


the naming of this function is misleading. if i see "find", i expect this to be read only to return me something that matches a search, but this is a function that mutates state. perhaps this shoudl be like "updateInstances" or "fetchInstances"

Agreed & updated

daywiss · 2024-07-22T15:44:28Z

src/lib/ovalDiscovery.ts

+
+        setTimeout(() => {
+            this.findOval(lastBlock);
+        }, env.ovalDiscoveryInterval * 1000);


this is surprising, and imo should be moved out to a higher level call to decide how to loop this

This class is a service that finds Oval instances deployed through the Factories.

This recursive function acts as a subscription to the events in the contracts. Instead of a subscription, we manually fetch the events every X minutes because event subscriptions sometimes cause issues with WebSockets closing or failing.

I think having the logic of the event retrieval contained in this class and abstracted away from the rest of the code makes it simple to reason about.

The initialization of the Oval discovery service is run in the index file at a higher level:

oval-node/src/index.ts

Lines 57 to 58 in fee339d

const ovalDiscovery = OvalDiscovery.getInstance();

ovalDiscovery.initialize(provider);

Knowing this: Would you extract the logic and handle the event retrieval process differently?

i would probably at the very least have a function that runs a singular update, and then have a separate call which runs that update in a loop continuously on the class, just to add a bit of composability and testability

daywiss · 2024-07-22T16:03:49Z

src/lib/walletManager.ts

+};
+
+// WalletManager class to handle wallet operations.
+export class WalletManager {


i dont really understand the logic behind how this manages the wallets, maybe need some comments

I've added some comments describing what each of the functions do

Signed-off-by: Pablo Maldonado <[email protected]>

Reinis-FRP

looking great!
just a nit comment on contract typings

Reinis-FRP · 2024-07-26T15:33:29Z

src/contract-types/PermissionProxy.ts

@@ -0,0 +1,249 @@
+/* Autogenerated file. Do not edit manually. */


Shouldn't these be gitignored and generated as part of build?

Also, for the sake of consistency, maybe we can also add Oval abi to this flow (currently its abi is in ts)

chrismaree · 2024-07-31T09:26:31Z

src/lib/events.ts

+        await Promise.map(paginatedRanges, ([fromBlock, toBlock]) => contract.queryFilter(filter, fromBlock, toBlock), {
+          concurrency: typeof searchConfig.concurrency == "number" ? searchConfig.concurrency : defaultConcurrency,
+        })
+      )
+        .flat()


did you need to use the bluebird promise due to limitations of the number of requests, that would error/fail without pagination?

I reused this existing code, used in other parts of our codebase without modifications to minimize impact. Additionally, the pagination that this has by default, I think, makes our usage of the node URLs efficient without adding much delay. Would you change it to normal Promises?

chrismaree · 2024-07-31T09:32:14Z

src/index.ts

+
+// Initialize Oval discovery
+const ovalDiscovery = OvalDiscovery.getInstance();
+ovalDiscovery.initialize(provider);


do we have any idea on how much of a performance impact this has (and all the underlying async calls this triggers) on startup times? given this is running in cloud run, is this problematic?

I did some tests, and querying 2 years of blocks in mainnet to find factories takes less than 10 seconds, querying 1 year takes 5.1 seconds. Since this only runs once when we restart the instance or when auto-scaling provisions a new instance, I think it’s fine. Do you agree?

Signed-off-by: Pablo Maldonado <[email protected]>

feat: oval multi unlocker

977b422

Signed-off-by: Pablo Maldonado <[email protected]>

md0x added 17 commits July 16, 2024 13:38

feat: add contract types

588cd5d

Signed-off-by: Pablo Maldonado <[email protected]>

feat: change structure and signatures

7efa89d

Signed-off-by: Pablo Maldonado <[email protected]>

feat: comments and env var

ddfd91e

Signed-off-by: Pablo Maldonado <[email protected]>

feat: ci tests

87bac83

Signed-off-by: Pablo Maldonado <[email protected]>

feat: fix tests

b3731f7

Signed-off-by: Pablo Maldonado <[email protected]>

feat: fix tests bis

cb52562

Signed-off-by: Pablo Maldonado <[email protected]>

fix: tests ci

9a5c3e2

Signed-off-by: Pablo Maldonado <[email protected]>

fix: tests ci pkeys

c092f09

Signed-off-by: Pablo Maldonado <[email protected]>

feat: simplify

de05da5

Signed-off-by: Pablo Maldonado <[email protected]>

feat: remove dep

ba6590b

Signed-off-by: Pablo Maldonado <[email protected]>

feat: init configs

c33b1de

Signed-off-by: Pablo Maldonado <[email protected]>

feat: remove extra type

d66cad8

Signed-off-by: Pablo Maldonado <[email protected]>

feat: clean tests

0aa2f98

Signed-off-by: Pablo Maldonado <[email protected]>

feat: oval discovery

4131b76

Signed-off-by: Pablo Maldonado <[email protected]>

feat: oval discovery improvements

9dfd12e

Signed-off-by: Pablo Maldonado <[email protected]>

feat: simplify call

9783f18

Signed-off-by: Pablo Maldonado <[email protected]>

feat: improve oval discovery initialization

7d2017b

Signed-off-by: Pablo Maldonado <[email protected]>

mrice32 reviewed Jul 18, 2024

View reviewed changes

md0x added 8 commits July 18, 2024 19:06

feat: data structure wallet manager

28ba4fc

Signed-off-by: Pablo Maldonado <[email protected]>

feat: improve wallet manager selection criteria

8773050

Signed-off-by: Pablo Maldonado <[email protected]>

feat: add log to notify if key used multiple times

6654052

Signed-off-by: Pablo Maldonado <[email protected]>

feat: tests use public getWallet

31d21c5

Signed-off-by: Pablo Maldonado <[email protected]>

feat: encode unlock for Permission Proxy

69b24fb

Signed-off-by: Pablo Maldonado <[email protected]>

feat: check that sender is correct

79fd454

Signed-off-by: Pablo Maldonado <[email protected]>

feat: update permission call

8ab9061

Signed-off-by: Pablo Maldonado <[email protected]>

feat: refine comments

f2fb908

Signed-off-by: Pablo Maldonado <[email protected]>

md0x marked this pull request as ready for review July 19, 2024 16:42

md0x requested a review from chrismaree as a code owner July 19, 2024 16:42

md0x requested review from Reinis-FRP and daywiss as code owners July 19, 2024 16:42

md0x requested a review from mrice32 July 19, 2024 16:42

md0x added 2 commits July 19, 2024 18:02

fix: stub permission proxy call

6bb034a

Signed-off-by: Pablo Maldonado <[email protected]>

feat: readme

34679cb

Signed-off-by: Pablo Maldonado <[email protected]>

daywiss approved these changes Jul 22, 2024

View reviewed changes

md0x added 4 commits July 23, 2024 11:07

feat: rename function and comments

d814d73

Signed-off-by: Pablo Maldonado <[email protected]>

feat: add try catch

5c77377

Signed-off-by: Pablo Maldonado <[email protected]>

feat: simplify factory update logic

fee339d

Signed-off-by: Pablo Maldonado <[email protected]>

Merge branch 'development' into pablo/oval-shared-unlockers

54d3ed1

Reinis-FRP approved these changes Jul 26, 2024

View reviewed changes

chrismaree reviewed Jul 31, 2024

View reviewed changes

md0x added 4 commits July 31, 2024 10:38

feat: remove contract types

9ef3966

Signed-off-by: Pablo Maldonado <[email protected]>

feat: ignore contract types

5a1c469

Signed-off-by: Pablo Maldonado <[email protected]>

feat: oval abi

fe07a88

Signed-off-by: Pablo Maldonado <[email protected]>

feat: oval discovery warm up log

35bf78e

Signed-off-by: Pablo Maldonado <[email protected]>

md0x merged commit aeb58ae into development Jul 31, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: update oval node to support oval factory deployments #83

feat: update oval node to support oval factory deployments #83

md0x commented Jul 15, 2024 •

edited

Loading

linear bot commented Jul 15, 2024

mrice32 left a comment

mrice32 Jul 18, 2024

md0x Jul 18, 2024

mrice32 Jul 18, 2024

mrice32 Jul 18, 2024

md0x Jul 18, 2024

daywiss Jul 22, 2024

md0x Jul 23, 2024

daywiss Jul 22, 2024

md0x Jul 23, 2024

daywiss Jul 23, 2024

daywiss Jul 22, 2024

md0x Jul 23, 2024

Reinis-FRP left a comment

Reinis-FRP Jul 26, 2024

chrismaree Jul 31, 2024

chrismaree Jul 31, 2024

md0x Jul 31, 2024

chrismaree Jul 31, 2024

md0x Jul 31, 2024

	const ovalDiscovery = OvalDiscovery.getInstance();
	ovalDiscovery.initialize(provider);

		@@ -0,0 +1,249 @@
		/* Autogenerated file. Do not edit manually. */

feat: update oval node to support oval factory deployments #83

feat: update oval node to support oval factory deployments #83

Conversation

md0x commented Jul 15, 2024 • edited Loading

linear bot commented Jul 15, 2024

mrice32 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

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

Reinis-FRP 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

md0x commented Jul 15, 2024 •

edited

Loading