sdk: allow delegates in single client #432

lowkeynicc · 2023-04-12T16:35:20Z

Tried to make this as backwards compatible as possible, consumers of the SDK shouldn't need to update unless they were accessing the driftClient.users map directly (instead of using something like driftClient.getUsers(). Basically with the possibility of delegate accounts sharing the same id as user accounts now, user map is now keyed off a string in the format of "[subAccountId]_[authority]"

Consumers will need to pass includeDelegates flag in the driftClientConfig (and to updateWallet) to have those accounts loaded and subscribed to. Can change this to load them by default if necessary

crispheaney · 2023-04-14T14:32:25Z

sdk/src/driftClient.ts

@@ -154,8 +155,8 @@ export class DriftClient {
 		);

 		this.authority = config.authority ?? this.wallet.publicKey;
-		const subAccountIds = config.subAccountIds ?? [0];
-		this.activeSubAccountId = config.activeSubAccountId ?? subAccountIds[0];
+		this.activeSubAccountId = config.activeSubAccountId;


why'd you get rid of the ?? [0]?

this way it will start undefined if not specified, then the call to addUsers will know no default is set and use the first user

i think this might break some backwards compatibility since not every user will call addUsers

crispheaney · 2023-04-14T14:34:08Z

sdk/src/driftClient.ts

-		this.createUsers(subAccountIds, this.userAccountSubscriptionConfig);
+
+		// load all or some users and add them to the driftClient
+		this.addAllUsers(


i dont think we want to do any async calls in the constructor, thats why the createUsers call didn't call subscribe before

maybe we have a new DriftClientConfig Param which is authoritySubaccountMap which allows you to specify the authority & subaccount pairs

crispheaney · 2023-04-14T14:35:50Z

sdk/src/tx/retryTxSender.ts

@@ -56,7 +56,7 @@ export class RetryTxSender implements TxSender {
 			opts = this.provider.opts;
 		}

-		const signedTx = preSigned
+		const signedTx = preSigned && tx.feePayer


if it's preSigned, why would it not have a feePayer? seems like there is an issue upstream?

no issue, just sort of a redundancy check but i can remove

crispheaney · 2023-04-14T14:44:20Z

sdk/src/driftClient.ts

-		const userAccounts = await this.getUserAccountsForAuthority(this.authority);
+	public async addAllUsers(
+		subAccountIds?: number[],
+		setFirstActive?: boolean,


when do you set this true versus false, trying to understand when we'd use it

at the moment if the user passes a subaccount id to the constructor or updateWallet it defaults the user to that, if not then this flag is true and it defaults to the first user

i feel like the constructor always needs to set the activeSubAccount for backwards compatability

crispheaney · 2023-04-14T14:44:50Z

sdk/src/driftClient.ts

 		for (const userAccount of userAccounts) {
-			await this.addUser(userAccount.subAccountId);
+			this.addUser(userAccount.subAccountId, this.authority);


not having await for these addUser will lead to race conditions?

didn't have any race condition issues with it and tested a lot, wanted to make it faster, will add back

crispheaney · 2023-04-14T14:45:25Z

sdk/src/driftClient.ts

+	public async addAllUsers(
+		subAccountIds?: number[],
+		setFirstActive?: boolean,
+		includeDelegates?: boolean


couldn't you just check that if the this.authority == wallet.pubkey to decide if the user has delegate set?

not sure what you mean by this, this flag indicates we want to load any/all of the accounts delegated to an authority as part of their subscribed driftclient users

yeah i don't think my comment made sense, nvm

crispheaney · 2023-04-14T14:47:46Z

sdk/src/driftClient.ts

-	public async addAllUsers(): Promise<void> {
-		const userAccounts = await this.getUserAccountsForAuthority(this.authority);
+	public async addAllUsers(
+		subAccountIds?: number[],


is this just for the constructor? would be nice if we didn't need to add this as it'd make the function less confusing

it's to allow them to only subscribe to a subset of subaccount ids

crispheaney · 2023-04-14T14:48:33Z

sdk/src/driftClient.ts

+		this.userStatsAccountPublicKey = undefined;
+
+		this.users.clear();
+		await this.addAllUsers(


why are you adding the users before the unsubscribe? feel like this is backwards?

mistake, not sure how it wasn't breaking anything

crispheaney · 2023-04-24T17:35:12Z

sdk/src/driftClient.ts

-		this.userStatsAccountPublicKey = undefined;
+		this.users.clear();
+
+		await this.addAndSubscribeToUsers(


why'd you move this out of the this.isSubscribed block? feel like it makes sense to only re-subscribe if you were already subscribed

i think isSubscribed would still be true from the driftclient being initialized?

this isSubscribed doesnt get set in the constructor, just if you call subscribe, so it could not be set? why'd you need to move this re-subscribe outta the if block?

crispheaney · 2023-04-24T17:36:59Z

sdk/src/driftClient.ts

-			this.accountSubscriber.subscribe()
-		);
+		let subscribePromises = [
+			this.addAndSubscribeToUsers(


all the parameters for this function exist on the DriftClient, do you need to pass em as parameters?

crispheaney · 2023-04-25T18:34:26Z

sdk/src/driftClientConfig.ts

@@ -20,6 +20,8 @@ export type DriftClientConfig = {
 	env?: DriftEnv;
 	userStats?: boolean;
 	authority?: PublicKey; // explicitly pass an authority if signer is delegate
+	includeDelegates?: boolean; // flag for whether to load delegate accounts as well
+	authoritySubaccountMap?: [string, number][]; // if passed this will override subAccountIds and includeDelegates


should it be a PublicKey instead of string

nice job adding comments 🙏

crispheaney · 2023-04-25T18:35:13Z

sdk/src/driftClient.ts

@@ -129,6 +130,9 @@ export class DriftClient {
 	authority: PublicKey;
 	marketLookupTable: PublicKey;
 	lookupTableAccount: AddressLookupTableAccount;
+	subAccountIds?: number[];
+	includeDelegates?: boolean;
+	authoritySubaccountMap?: [string, number][];


I'm thinking we only have authoritySubaccountMap and get rid of the subAccountIds on the actual DriftClient. If the user passes the subAccounts, we just default to creating the map for them given the authority?

crispheaney · 2023-04-25T18:36:51Z

sdk/src/driftClient.ts

-		this.userStatsAccountPublicKey = undefined;
+		this.users.clear();
+
+		await this.addAndSubscribeToUsers(


this isSubscribed doesnt get set in the constructor, just if you call subscribe, so it could not be set? why'd you need to move this re-subscribe outta the if block?

crispheaney · 2023-04-25T18:44:10Z

sdk/src/driftClient.ts

-		const userAccounts = await this.getUserAccountsForAuthority(this.authority);
-		for (const userAccount of userAccounts) {
-			await this.addUser(userAccount.subAccountId);
+	public async addAndSubscribeToUsers(): Promise<boolean> {


For this, i'm thinking if we've made the change to just have the DriftClient to only have the map:

if the map isn't empty, add the users based on the authority + sub account ids in the map

if it is empty, make the call to getUserAccountsForAuthority/getUserAccountsForDelegates

I think this is desierable because the calls to getUserAccountsForAuthority and getUserAccountsForDelegate make getProgramAccount rpc calls, which can be expensive, so good to avoid em if we can

crispheaney · 2023-04-26T14:05:25Z

sdk/src/driftClient.ts

-		const subAccountIds = config.subAccountIds ?? [0];
-		this.activeSubAccountId = config.activeSubAccountId ?? subAccountIds[0];
+		this.activeSubAccountId = config.activeSubAccountId ?? 0;
+		this.authoritySubaccountMap = config.authoritySubaccountMap ?? undefined;


think you can get rid of this since you reset it below

crispheaney · 2023-04-26T14:08:21Z

sdk/src/driftClient.ts

+			? config.authoritySubaccountMap
+			: config.subAccountIds
+			? config.subAccountIds.map((acctId) => [this.authority, acctId])
+			: undefined;


is it possible we just set it as empty if they don't pass anything? all else equal, nice to avoid undefined/null

crispheaney · 2023-04-26T14:10:13Z

sdk/src/driftClient.ts

+			for (const account of userAccounts.concat(delegatedAccounts)) {
+				result =
+					result &&
+					(await this.addUser(account.subAccountId, account.authority));


we made it so you can pass in a UserAccount object to user.subscribe to speed up the subscribe logic, we should probably use that here since we already have all the UserAccounts (we can do this in a separate pr later)

crispheaney · 2023-04-26T14:14:19Z

sdk/src/driftClient.ts

@@ -129,6 +130,8 @@ export class DriftClient {
 	authority: PublicKey;
 	marketLookupTable: PublicKey;
 	lookupTableAccount: AddressLookupTableAccount;
+	includeDelegates?: boolean;
+	authoritySubaccountMap?: [PublicKey, number][];


sorry last nit pit, should this be a map instead of an array of arrays? if you wanted multiple sub accounts from same pub key, you'd have to do something like:

[ [Pubkey.default(), 0], [Pubkey.default(), 1], [Pubkey.default(), 2], ]

with a map it'd be:

new Map([ [Pubkey.default(), [0, 1, 2]] ])

(I think it'd be to be Map<string, number[]> since PublicKey doesnt really work as a key)

lowkeynicc added 5 commits April 10, 2023 22:13

sdk: updates for delegate wip

4c02ea4

save

978fd42

wip

2406bad

updates

bd70516

updates

8ef24c1

crispheaney reviewed Apr 14, 2023

View reviewed changes

lowkeynicc added 6 commits April 14, 2023 11:15

feedback

7a61cc0

updates

25438f3

merge

96ec680

merge conflict

b19cdd1

updates

e931f2e

sdk: fix userStats account

bf06214

crispheaney reviewed Apr 24, 2023

View reviewed changes

lowkeynicc added 3 commits April 24, 2023 22:24

add authoritySubaccountMap

615d220

fix

24d651b

disallow subaccountmap and includedelegates

aee3d1b

crispheaney reviewed Apr 25, 2023

View reviewed changes

lowkeynicc added 2 commits April 25, 2023 17:10

more feedback

1200b45

update to work for user with only delegate accounts

a706d73

crispheaney reviewed Apr 26, 2023

View reviewed changes

lowkeynicc added 5 commits April 26, 2023 16:15

updates to authoritysubaccountmap

68c1ac1

update changelog + fix default to delegate acct

791f2d4

merge

91c4bc6

fix prettier

9d74c0c

add skipAddUsers flag

dca79b5

lowkeynicc merged commit 294b832 into master Apr 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sdk: allow delegates in single client #432

sdk: allow delegates in single client #432

lowkeynicc commented Apr 12, 2023

crispheaney Apr 14, 2023

lowkeynicc Apr 14, 2023

crispheaney Apr 17, 2023

crispheaney Apr 14, 2023

crispheaney Apr 14, 2023

crispheaney Apr 14, 2023

lowkeynicc Apr 14, 2023

crispheaney Apr 14, 2023

lowkeynicc Apr 14, 2023

crispheaney Apr 17, 2023

crispheaney Apr 14, 2023

lowkeynicc Apr 14, 2023

crispheaney Apr 14, 2023

lowkeynicc Apr 14, 2023

crispheaney Apr 17, 2023

crispheaney Apr 14, 2023

lowkeynicc Apr 14, 2023

crispheaney Apr 14, 2023

lowkeynicc Apr 14, 2023

crispheaney Apr 24, 2023

lowkeynicc Apr 25, 2023 •

edited

Loading

crispheaney Apr 25, 2023

crispheaney Apr 24, 2023

crispheaney Apr 25, 2023

crispheaney Apr 25, 2023

crispheaney Apr 25, 2023

crispheaney Apr 25, 2023

crispheaney Apr 26, 2023

crispheaney Apr 26, 2023

crispheaney Apr 26, 2023

crispheaney Apr 26, 2023

sdk: allow delegates in single client #432

sdk: allow delegates in single client #432

Conversation

lowkeynicc commented Apr 12, 2023

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

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

lowkeynicc Apr 25, 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

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

lowkeynicc Apr 25, 2023 •

edited

Loading