l1 chain #1219

benny-conn · 2023-10-05T19:18:25Z

Changes:

Added a new l1_chain column to wallets and nonces table.
Changed many queries to search by l1_chain
Changed WalletOverrides to live in persist instead of wire injection

benny-conn · 2023-10-05T22:31:31Z

db/migrations/core/000111_l1_chain.up.sql

+create index nonces_l1_chain_idx on nonces (address,chain,l1_chain);
+create index wallets_l1_chain_idx on wallets (address,chain,l1_chain);
+create unique index wallets_l1_chain_unique_idx on wallets (address,l1_chain) where deleted = false;


Do we need to remove any of the current indexes, specifically the unique ones on (address,chain)? I imagined we still would want them because we still need those two to be unique (despite them always being unique given we correctly use the l1_chain, e.g. there will be no case where two rows with (addr: 0x9, ch: BASE) and (addr:0x9, ch: BASE) getting through because they will always fail on (addr: 0x9, l1_ch: ETH) anyway). I imagine leaving them would not hurt us.

I'm in favor of leaving it for now! I think your point is spot-on: we shouldn't need it, assuming we're getting the L1 chain correct...but the wallets table is relatively small, the extra index doesn't cost us much, and I like the peace of mind in knowing that we can't accidentally end up in a situation that should be impossible.

benny-conn · 2023-10-05T22:33:43Z

service/persist/postgres/wallet.go

-// Insert inserts a wallet by its address and chain
-func (w *WalletRepository) Insert(ctx context.Context, chainAddress persist.ChainAddress, walletType persist.WalletType) (persist.DBID, error) {
-
-	_, err := w.insertStmt.ExecContext(ctx, persist.GenerateID(), 0, chainAddress.Address(), chainAddress.Chain(), walletType)
-	if err != nil {
-		return "", err
-	}
-
-	// rather than using the ID generated above, we must retrieve it because in the case of conflict the ID above would be inaccurate.
-	wa, err := w.GetByChainAddress(ctx, chainAddress)
-	if err != nil {
-		return "", err
-	}
-
-	return wa.ID, nil
-}


benny-conn · 2023-10-05T22:33:57Z

sqlc.yaml

-          - column: '*.*.chain'
-            go_type: 'github.com/mikeydub/go-gallery/service/persist.Chain'


Duplicated above

radazen

Looks awesome, Benny! I really like how this turned out, and I think it's going to simplify a lot of the L1 chain issues we've been running into!

radazen · 2023-10-10T13:57:14Z

db/migrations/core/000111_l1_chain.up.sql

+update wallets set l1_chain = 4 where chain = 4;
+
+create index nonces_l1_chain_idx on nonces (address,chain,l1_chain);
+create index wallets_l1_chain_idx on wallets (address,chain,l1_chain);


I think the non-unique indexes here would also benefit from where deleted = false

radazen · 2023-10-10T14:07:46Z

db/migrations/core/000111_l1_chain.up.sql

+create index nonces_l1_chain_idx on nonces (address,chain,l1_chain);
+create index wallets_l1_chain_idx on wallets (address,chain,l1_chain);
+create unique index wallets_l1_chain_unique_idx on wallets (address,l1_chain) where deleted = false;


I'm in favor of leaving it for now! I think your point is spot-on: we shouldn't need it, assuming we're getting the L1 chain correct...but the wallets table is relatively small, the extra index doesn't cost us much, and I like the peace of mind in knowing that we can't accidentally end up in a situation that should be impossible.

radazen · 2023-10-10T14:19:49Z

db/queries/core/query.sql

-  and w.id = any(u.wallets) and coalesce(nullif(c.owner_address, ''), nullif(c.creator_address, '')) = w.address
-  and c.chain = w.chain
+  and w.id = any(u.wallets) and coalesce(nullif(c.owner_address, ''), nullif(c.creator_address, '')) = w.address 
+  and w.chain = any(@l1_chains::int[])


Hmm, this makes me think we should add the l1_chain column to the contracts table, too. That way, we can say "give me contracts on any(@chains) where the user owns a wallet on the contract's L1 chain," which is (I think) the most correct way to phrase this query.

...but also, that would add some pretty significant scope to this PR, and I don't think it's necessary to add it for the sake of this one query. I think your current workaround is good here, but we should definitely add l1_chain to contracts in the near future!

Agreed, was thinking this as well when I had to make this little work around. If we do contracts, might as well do tokens too at some point...

radazen · 2023-10-10T14:34:02Z

service/persist/auth.go

@@ -22,6 +22,8 @@ type UserNonce struct {
 	LastUpdated  time.Time  `json:"last_updated"`
 	Value        NullString `json:"value"`
 	Address      Address    `json:"address"`
+	Chain        Chain      `json:"chain"`
+	L1Chain      Chain      `json:"l1_chain"`


Could we add a new type for L1Chain instead of repurposing Chain? IMHO we wouldn't want to accept an L1Chain anywhere we'd accept a regular Chain, or vice versa.

radazen · 2023-10-10T14:41:19Z

service/persist/postgres/wallet.go

 	checkNoErr(err)

-	getByIDStmt, err := db.PrepareContext(ctx, `SELECT ID,VERSION,CREATED_AT,LAST_UPDATED,ADDRESS,WALLET_TYPE,CHAIN FROM wallets WHERE ID = $1 AND DELETED = FALSE;`)
+	getByChainAddressStmt, err := db.PrepareContext(ctx, `SELECT ID,VERSION,CREATED_AT,LAST_UPDATED,ADDRESS,WALLET_TYPE,CHAIN,L1_CHAIN FROM wallets WHERE ADDRESS = $1 AND L1_CHAIN = $2 AND DELETED = FALSE;`)


Do we need the concept of an L1ChainAddress that uses L1Chain instead of a regular chain? It's starting to feel that way to me! A query named getByChainAddress makes me think we'll be passing in a Chain, but we're actually using an L1Chain here, and it'd probably make the codebase clearer if we separated those concepts.

l1 chain

72a52bf

benny-conn requested review from radazen and jarrel-b as code owners October 5, 2023 19:18

github-actions bot added migration query labels Oct 5, 2023

benny-conn added 4 commits October 5, 2023 12:25

l1 chain group

a237fc6

merge and indicies

cd2faf9

check l1

c6826b4

batch

7c13f2b

github-actions bot added the dataloader label Oct 5, 2023

benny-conn added 4 commits October 5, 2023 13:20

data loaders

ad582d6

no merge because unused anyway

2158fa8

correct test chains

7cb74e2

not null constraint

68c0008

benny-conn commented Oct 5, 2023

View reviewed changes

benny-conn added 3 commits October 5, 2023 15:35

use stmt still

89c6bac

Merge branch 'main' into benny/l1-chain

18ea085

l1 chain check for creators

f6b0528

radazen approved these changes Oct 10, 2023

View reviewed changes

benny-conn added 2 commits October 10, 2023 14:05

l1 chain and chain address

a4d6a04

contract chain address

3ecce6b

benny-conn merged commit 77bb22b into main Oct 10, 2023
6 checks passed

benny-conn deleted the benny/l1-chain branch October 10, 2023 21:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

l1 chain #1219

l1 chain #1219

benny-conn commented Oct 5, 2023

benny-conn Oct 5, 2023

radazen Oct 10, 2023

benny-conn Oct 5, 2023

benny-conn Oct 5, 2023

radazen left a comment

radazen Oct 10, 2023

radazen Oct 10, 2023

radazen Oct 10, 2023 •

edited

Loading

benny-conn Oct 10, 2023

radazen Oct 10, 2023

benny-conn Oct 10, 2023

radazen Oct 10, 2023

benny-conn Oct 10, 2023

		- column: '..chain'
		go_type: 'github.com/mikeydub/go-gallery/service/persist.Chain'

l1 chain #1219

l1 chain #1219

Conversation

benny-conn commented Oct 5, 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

radazen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radazen Oct 10, 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

radazen Oct 10, 2023 •

edited

Loading