Adding createDao() method #8

lucaslencinas · 2022-10-04T15:34:27Z

Description

Adding the createDao() method.

Basically, doing the same steps that are done across several repositories in one place.
In Nation, Oyster UI, Governance UI, Marinade and probably other places there are lots of duplicated code in order to create a simple MultiSig wallet.
With this PR, any project should be able to do just:

const dao = await client.createDao(members, name, yesVoteThreshold)

Also:

Created a daoService using a daoRepository and a tokenRepository in order to split the responsibility.
Created a TestWallet

Things still to do:

Unify all the transactions into one. I'd rather do that in another PR. Not a blocker for me for now.

packages/solana-dao-sdk/src/index.test.ts

packages/react-app-example-daos/package.json

packages/react-app-example-daos/config-overrides.js

packages/solana-dao-sdk/src/index.test.ts

packages/solana-dao-sdk/src/internal/realms.ts

arielsegura · 2022-10-04T20:06:16Z

packages/solana-dao-sdk/src/index.test.ts

+    const daoName = `dao ${randomName}`;
+
+    const someDao = await client.createDao(
+      {


I know it's draft but I don't want to forget this:

It would be nice if the method createDao uses the connection in this (we pass it over in the constructor), so that we always use the same connection in every call.

Program ID could be a default value, same as the program version

We can set the user wallet with a setter after the user connects their wallet or pass it in the constructor if we have it

If we pass userWallet, we can infer userWallet.publicKey in the method

arielsegura · 2022-10-04T20:07:07Z

packages/solana-dao-sdk/src/index.ts

@@ -23,12 +26,63 @@ export type Dao = {
  votingProposalCount: number;
 };

+export class TestWallet {


I definitely see this TestWallet being reused in other test cases, should we move it to the test directory?

also, is it implementing some other wallet interface? it would be good to annotate with "extends" or "implements" in that case

arielsegura · 2022-10-04T20:11:50Z

packages/solana-dao-sdk/src/internal/realms.ts

+const tokenAmount = 1;
+
+export async function createMultisigRealm1(
+  connection: Connection,


to avoid passing the connection, wallet, programId and program version everywhere, we could migrate the method createMultisigRealm as a public method in an internal object/class like DAORepository or DAOStorage, or something that indicates input/output, and pass the parameters I mentioned in the constructor. Then createMint, and all creations can be private methods in the same object.

arielsegura · 2022-10-04T20:14:52Z

packages/solana-dao-sdk/src/internal/realms.ts

+    );
+
+    if (communityMint.signers.length > 0) {
+      communityMintTx.partialSign(...communityMint.signers);


what does communityMint.signers contain in this context? just wallet.publicKey or some new public keys?

arielsegura · 2022-10-04T20:16:35Z

packages/solana-dao-sdk/src/internal/realms.ts

+      feePayer: wallet.publicKey,
+    });
+
+    const councilsMintTx = new Transaction({


to improve readibility, lets move this down to line ~92 right before we need it

arielsegura · 2022-10-04T20:17:37Z

packages/solana-dao-sdk/src/internal/realms.ts

+      walletPk
+    );
+
+    // transaction signatures here for now


to improve readability, let's move this comment to line 102, right where it's relevant

Also, do you have any thoughts about what could be a better place for the signatures? If yes, I'd transform this comment either in a ticket or a TODO saying what to do. If not, I'd just remove the comment as we won't do anything with it in the near future and for now will be forever :D

arielsegura · 2022-10-04T20:21:43Z

packages/solana-dao-sdk/src/internal/realms.ts

+
+    // signature.toString('base64')
+
+    const recentBlockhash1 = await connection.getLatestBlockhash();


why another blockchash? could we not use just one?

packages/solana-dao-sdk/src/internal/realms.ts

arielsegura · 2022-10-04T20:32:35Z

packages/solana-dao-sdk/src/internal/realms.ts

+    } else {
+      // Let's throw for now if the current wallet isn't in the team
+      // TODO: To fix it we would have to make it temp. as part of the team and then remove after the realm is created
+      throw new Error("Current wallet must be in the team");


So what happens if we reach this line? I'm guessing the DAO is partially created as we already signed and sent a few transactions, but others below this line were never executed.

Is it possible for us to fail fast? i.e. do this check before even signing a transaction

arielsegura · 2022-10-04T20:35:51Z

packages/solana-dao-sdk/src/internal/realms.ts

+
+    realmInstructions.forEach((instruction) => realmRelatedTx.add(instruction));
+
+    const signedTxs3 = await wallet.signAllTransactions([realmRelatedTx]);


have you tried signing and sending all transactions at the same time? i.e. generating all the instructions first and then packing them all (in the right order) in the same transaction

For some reason I'm still having issues with that. If I cannot fix it quite fast, I'll do it in a following PR

packages/solana-dao-sdk/src/internal/sdk/splToken/withCreateAssociatedTokenAccount.ts

arielsegura · 2022-10-04T20:48:47Z

const dao = await client.createDao(connetion, wallet, members, name, yesVoteThreshold)

this could be simplified to

await client.createDao(wallet, members, name, yesVoteThreshold)

as the connection is in the client already (this.connection).

Even further:

// when we detect that the user connected their wallet
client.setWallet(wallet);
...
// yesVoteThreshold: default to 0.5? or 0.6? I think I read somewhere what a nice default value is. And we can change it anytime no? 
// members: default to single member, `wallet.publicKey`
// name mandatory with no default value. Sounds like the most important parameter
await client.createDao(name)

// whenever we detect that the user disconnected their wallet
client.setWallet(undefined)

…e codebase

lucaslencinas · 2022-10-09T22:49:38Z

This is ready for another review. @arielsegura

arielsegura · 2022-10-11T19:51:11Z

packages/react-app-example-daos/README.md

+## Moving to react-app-rewired
+
+Due to a couple of issues we started using create-app-rewired.
+This is because CRA doesn't handle .cjs files and inside the spl-governance dependecy tree we have a few of them when compiled.


Suggested change

This is because CRA doesn't handle .cjs files and inside the spl-governance dependecy tree we have a few of them when compiled.

This is because CRA doesn't handle .cjs files and inside the spl-governance dependency tree we have a few of them when compiled.

arielsegura

It looks really really good, good job!

I left some non blocking comments (nit) that we can discuss later or fix in other PRs, some other comments would be nice to address them now, but we can also discuss that :D

🎉

arielsegura · 2022-10-11T19:53:22Z

packages/react-app-example-daos/src/providers/DaoProvider.tsx

  return (
-    <DaoContext.Provider value={new SolanaDao()}>
+    <DaoContext.Provider value={new SolanaDao(devnetConnection)}>


if you are changing this because of the too many requests problem, devnet can have the same problem. I think it would be good to use mainnet by default, even on the example. Devnet is usually very unstable.

For rendering DAOs yeah, we could use prod, yes. But now that we have the createDao() would you want to spend real SOL for trying out things?

Also, we can have a query parameter like most products do to indicate which network tu use

For rendering DAOs yeah, we could use prod, yes. But now that we have the createDao() would you want to spend real SOL for trying out things?

ideally, we should integrate with a wallet that would enable the user to make this decision. I'm suggesting mainnet-beta because the first interaction that "testers" will have is reading, not writing. If this first interaction fails (because devnet is unstable), I really doubt they will try writing

arielsegura · 2022-10-11T19:55:43Z

packages/solana-dao-sdk/src/constants.ts

+
+export const governanceProgramVersion = 2;
+export const governancePk = new PublicKey(
+  "GovER5Lthms3bLBqWub97yVrMmEogzX7xNjdXpPPCVZw"


this governancePk is the same as the constant in index.ts. Let's have it in only one place

There is only one now 🤔

arielsegura · 2022-10-11T20:00:17Z

packages/solana-dao-sdk/src/index.test.ts

+    );
+
+    // So we can check on the Explorer how it looks.


nit: what exactly do you look for in the explorer? I wonder if we can call getDao to make a few assertions instead?

packages/solana-dao-sdk/src/index.ts

packages/solana-dao-sdk/src/internal/tokens.ts

packages/solana-dao-sdk/src/test/utils.ts

packages/solana-dao-sdk/src/wallet.ts

lucaslencinas force-pushed the createDao-first-version branch from 4b5a369 to 9ed5e70 Compare October 4, 2022 15:48