MWA 2.0 Auth Repository #605

Funkatronics · 2023-11-13T23:07:16Z

Updates the walletlib AuthDatabase to store new authorized account parameters (account icon, chains, features). Updates the database schema version from 5>6 and handles database migration.

This PR does not address multiple authorized accounts per auth token. those changes got too complex and will come in a future PR.

Testing database migration:

clean installed fakewallet built from v1.x branch
initiated several authorizations from fakedapp to fill the auth repository
upgraded fakewallet with build form this branch
verified that auth records were successfully migrated (using SQLiteDatabaseBrowser)

...c/main/java/com/solana/mobilewalletadapter/walletlib/authorization/AccountRecordsSchema.java

Michaelsulistio · 2023-11-21T00:15:03Z

...c/main/java/com/solana/mobilewalletadapter/walletlib/authorization/AuthorizationsSchema.java

@@ -9,7 +9,7 @@
    String COLUMN_AUTHORIZATIONS_ID = "id"; // type: int
    String COLUMN_AUTHORIZATIONS_IDENTITY_ID = "identity_id"; // type: long
    String COLUMN_AUTHORIZATIONS_ISSUED = "issued"; // type: long
-    String COLUMN_AUTHORIZATIONS_PUBLIC_KEY_ID = "public_key_id"; // type: long
+    String COLUMN_AUTHORIZATIONS_ACCOUNT_ID = "public_key_id"; // type: long


Is COLUMN_AUTHORIZATIONS_ACCOUNT_ID value suppose to be "public_key_id"? It's unchanged from the previous schema.

yes, good callout. this made it easy(easier) to migrate, as I don't need to migrate this column. This column will be changed again anyways when we move to multi account.

ah ya know, now that I think about this more, I might not be handling migration of these ids correctly...

I am only migrating the publickey records over to the new AccountRecord but I do not actually update these ids. hmm. will get this fixed asap

Summarizing my understanding of the issue:

Pre-migrated databases will have an Authorizations Table with rows that contain a column (public_key_id) that contains a publicKeyId. When the migration step (onUpgrade) occurs, the Authorizations Table is not updated and the public_key_id column will still contain publicKeyIds rather than accountIds.

This is problematic because certain parts of code will assume that the column contains accountIds when they are actually still publicKeyIds. I'll add comments for spots where I see this being problematic.

Solutions:

Create a new column for account_id in Authorizations Table then populate it during the migration step onUpgrade. Migrated DBs will just have a redundant public_key_id column that isn't used anymore.

On upgrade/migration, alter each row in Authorizations Table so that public_key_id == account_id, by editing/updating each public_key_id value. (Is it possible to change a row's id? Where is the row's id even determined?).

The names of the columns does not really matter, as long as the name is respected in our code (the schema is not exposed publicly).

we cant drop a column in SQLite but we can rename it. So rather than deleting the table and recreating it with the new schema, I opted to rename the public_key_id column to account_id then migrate the ids over. its maybe not the most ideal solution but it should work okay.

I don't think we can change the primary key of a row in SQL. If we did, we'd also need to update all of the references on that ID. So I instead just updated the stored id for auth authorization record.

Michaelsulistio · 2023-11-21T00:17:36Z

...etlib/src/main/java/com/solana/mobilewalletadapter/walletlib/authorization/AuthDatabase.java

@@ -29,18 +35,35 @@ public void onConfigure(SQLiteDatabase db) {
    public void onCreate(SQLiteDatabase db) {
        db.execSQL(IdentityRecordSchema.CREATE_TABLE_IDENTITIES);
        db.execSQL(AuthorizationsSchema.CREATE_TABLE_AUTHORIZATIONS);
-        db.execSQL(PublicKeysSchema.CREATE_TABLE_PUBLIC_KEYS);
+        db.execSQL(AccountRecordsSchema.CREATE_TABLE_ACCOUNTS);
        db.execSQL(WalletUriBaseSchema.CREATE_TABLE_WALLET_URI_BASE);
    }

    @Override
    public void onUpgrade(SQLiteDatabase db, int oldVersion, int newVersion) {


When does onUpgrade trigger? Or, in other words, how does walletlib know when to onUpgrade the database?

onUpgrade is called whenever the database schema version changes (5->6 in this case)

Michaelsulistio · 2023-11-21T00:20:50Z

...c/main/java/com/solana/mobilewalletadapter/walletlib/protocol/MobileWalletAdapterServer.java

-        @NonNull @Size(min = 1)
-        public final AuthorizedAccount[] accounts;
+        @NonNull
+        public final AuthorizedAccount account;


Summarizing my understanding:

We're going from AuthorizedAccount[] to AuthorizedAccount because only the first item of the array was ever used anyways. Change it to a single account for clarity (until its actually implemented).

in v1.x, this was never even an array, just a single public key and account label. the AuthorizedAccount struct was added in the 2.0 upgrade. This is reverting some of the original 2.0 changes to be single account tho for clarity, yes.

Michaelsulistio · 2023-11-21T00:22:12Z

.../src/main/java/com/solana/mobilewalletadapter/walletlib/authorization/AccountRecordsDao.java

+import androidx.annotation.NonNull;
+import androidx.annotation.Nullable;
+
+public class AccountRecordsDao extends DbContentProvider<AccountRecord>


Summarizing my understanding:

We're changing our "account" model from PublicKeyRecord -> AccountRecord because in MWA 2.0, the "account" model needs to be expanded to include chains/features/etc. A public key is now a part of the larger AccountRecord model.

yep exactly. adding new account metadata fields to the database.

having this already done will make it easier to migrate to multi-account too. we still need to store a list of AccountRecords for each authorization, but at least now the AccountRecord itself is there.

Michaelsulistio

Had some more time to read through the changes and have a better understanding now. Left my reasoning in the comments, but please correct me if I have a misunderstanding!

Michaelsulistio · 2023-11-24T21:05:56Z

.../src/main/java/com/solana/mobilewalletadapter/walletlib/authorization/AuthorizationsDao.java

-                        " = " + TABLE_PUBLIC_KEYS + '.' + COLUMN_PUBLIC_KEYS_ID +
+                        " INNER JOIN " + TABLE_ACCOUNTS +
+                        " ON " + TABLE_AUTHORIZATIONS + '.' + COLUMN_AUTHORIZATIONS_ACCOUNT_ID +
+                        " = " + TABLE_ACCOUNTS + '.' + COLUMN_ACCOUNTS_ID +


getAuthorizations() is incorrect if COLUMN_AUTHORIZATIONS_ACCOUNT_ID still refers to a publicKeyId.

The INNER_JOIN will try to match COLUMN_AUTHORIZATIONS_ACCOUNT_ID == COLUMN_ACCOUNTS_ID, but COLUMN_AUTHORIZATIONS_ACCOUNT_ID columns will all be publicKeyId values while its expecting accountId values. This results in an empty join.

fixed with migration from public_key_id column to account_id

Michaelsulistio · 2023-11-24T21:19:02Z

...c/main/java/com/solana/mobilewalletadapter/walletlib/authorization/AuthorizationsSchema.java

@@ -9,7 +9,7 @@
    String COLUMN_AUTHORIZATIONS_ID = "id"; // type: int
    String COLUMN_AUTHORIZATIONS_IDENTITY_ID = "identity_id"; // type: long
    String COLUMN_AUTHORIZATIONS_ISSUED = "issued"; // type: long
-    String COLUMN_AUTHORIZATIONS_PUBLIC_KEY_ID = "public_key_id"; // type: long
+    String COLUMN_AUTHORIZATIONS_ACCOUNT_ID = "public_key_id"; // type: long


Summarizing my understanding of the issue:

Pre-migrated databases will have an Authorizations Table with rows that contain a column (public_key_id) that contains a publicKeyId. When the migration step (onUpgrade) occurs, the Authorizations Table is not updated and the public_key_id column will still contain publicKeyIds rather than accountIds.

This is problematic because certain parts of code will assume that the column contains accountIds when they are actually still publicKeyIds. I'll add comments for spots where I see this being problematic.

Solutions:

Create a new column for account_id in Authorizations Table then populate it during the migration step onUpgrade. Migrated DBs will just have a redundant public_key_id column that isn't used anymore.

On upgrade/migration, alter each row in Authorizations Table so that public_key_id == account_id, by editing/updating each public_key_id value. (Is it possible to change a row's id? Where is the row's id even determined?).

Michaelsulistio · 2023-11-24T21:20:55Z

.../src/main/java/com/solana/mobilewalletadapter/walletlib/authorization/AccountRecordsDao.java

+
+    @Override
+    public void deleteUnreferencedAccounts() {
+        final SQLiteStatement deleteUnreferencedPublicKeys = super.compileStatement(


deleteUnreferencedPublicKeys should be renamed to deleteUnreferencedAccounts?

Michaelsulistio · 2023-11-24T21:23:49Z

.../src/main/java/com/solana/mobilewalletadapter/walletlib/authorization/AccountRecordsDao.java

+        final SQLiteStatement deleteUnreferencedPublicKeys = super.compileStatement(
+                "DELETE FROM " + TABLE_ACCOUNTS +
+                        " WHERE " + COLUMN_ACCOUNTS_ID + " NOT IN " +
+                        "(SELECT DISTINCT " + AuthorizationsSchema.COLUMN_AUTHORIZATIONS_ACCOUNT_ID +


Another problematic spot for migrated DBs in deleteUnreferencedAccounts.

COLUMN_ACCOUNTS_ID is expecting accountIds but AuthorizationsSchema.COLUMN_AUTHORIZATIONS_ACCOUNT_ID are still publicKeyIds.

fixed with migration from public_key_id column to account_id

Funkatronics added 2 commits November 13, 2023 16:03

auth repo changes only

6ffd522

single account auth only with new auth repo

daeb2df

Funkatronics commented Nov 14, 2023

View reviewed changes

...c/main/java/com/solana/mobilewalletadapter/walletlib/authorization/AccountRecordsSchema.java Show resolved Hide resolved

Funkatronics added 2 commits November 14, 2023 09:51

cleanup

b6b9792

remove debug print

297ab98

Funkatronics changed the title ~~[WIP] MWA 2.0 Auth Repository~~ MWA 2.0 Auth Repository Nov 14, 2023

Funkatronics requested review from creativedrewy, sdlaver and Michaelsulistio and removed request for creativedrewy November 14, 2023 17:03

Michaelsulistio reviewed Nov 21, 2023

View reviewed changes

Michaelsulistio approved these changes Nov 21, 2023

View reviewed changes

Michaelsulistio reviewed Nov 24, 2023

View reviewed changes

Funkatronics added 4 commits November 27, 2023 10:01

fix db migration

781aec4

cleanup

f44a4df

pr feedback - rename query

872af79

add comment

027bebe

Funkatronics merged commit 5622b1c into main Nov 27, 2023
5 checks passed

Funkatronics deleted the mwa-2.0-auth-repository branch November 27, 2023 21:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MWA 2.0 Auth Repository #605

MWA 2.0 Auth Repository #605

Funkatronics commented Nov 13, 2023 •

edited

Loading

Michaelsulistio Nov 21, 2023

Funkatronics Nov 21, 2023

Funkatronics Nov 21, 2023

Michaelsulistio Nov 24, 2023

Funkatronics Nov 27, 2023

Michaelsulistio Nov 21, 2023

Funkatronics Nov 21, 2023

Michaelsulistio Nov 21, 2023

Funkatronics Nov 21, 2023

Michaelsulistio Nov 21, 2023

Funkatronics Nov 21, 2023

Michaelsulistio left a comment

Michaelsulistio Nov 24, 2023

Funkatronics Nov 27, 2023

Michaelsulistio Nov 24, 2023

Michaelsulistio Nov 24, 2023

Funkatronics Nov 27, 2023

Michaelsulistio Nov 24, 2023

Funkatronics Nov 27, 2023

MWA 2.0 Auth Repository #605

MWA 2.0 Auth Repository #605

Conversation

Funkatronics commented Nov 13, 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

Michaelsulistio 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

Funkatronics commented Nov 13, 2023 •

edited

Loading