-
Notifications
You must be signed in to change notification settings - Fork 8
feat: implement creation wallet with fireblocks #824
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: implement creation wallet with fireblocks #824
Conversation
Signed-off-by: Alina Radyukevich <[email protected]>
Signed-off-by: Alina Radyukevich <[email protected]>
api-specs/openrpc-user-api.json
Outdated
| } | ||
| }, | ||
| "required": ["wallet"] | ||
| "$ref": "#/components/schemas/Null" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can and should return a Wallet. For signing providers internal and participant, we should leave the code unchanged, and for fireblocks we can still create and return a wallet but just set the status accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or is it because the partyId is not known yet due to a missing public key, from which the namespace (right side of the partyId) is derived?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I think I’ll be able to bring that logic back
api-specs/openrpc-user-api.json
Outdated
| "description": "Allocates a new wallet and party with the given hint." | ||
| }, | ||
| { | ||
| "name": "allocateWallet", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first glimpse, I don't think this extra api method is necessary. externalTxId and topologyTransactions could just be properties of an additional object CreateWalletParams.signingProviderContext.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's more convenient to separate the logic the way I did now, because they perform slightly different actions. But if you feel it's better to combine them into one, I can refactor it.
api-specs/openrpc-user-api.json
Outdated
| "type": "string", | ||
| "description": "The signing provider ID the wallet corresponds to." | ||
| }, | ||
| "txId": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
externalTxId
api-specs/openrpc-user-api.json
Outdated
| "type": "string", | ||
| "description": "The transaction ID" | ||
| }, | ||
| "transactions": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
topologyTransactions
| SELECT party_id, "primary", hint, public_key, "namespace", chain_id, signing_provider_id FROM wallets | ||
| `.execute(db) | ||
|
|
||
| await db.schema.dropTable('wallets').execute() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This removes an existing wallet table. The migrations are meant to contain incremental changes such as adding a field to an existing table. Given that your fields are nullable, I think this should actually work here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides, did you think of potentially creating a separate table for the fireblocks related additional information?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm creating a new table called wallets_temp, where party_id will no longer be the primary key and an auto-incrementing ID will be used instead. Then I copy all the data from the wallets table into wallets_temp. Once all the data has been copied, we can delete the old wallets table and rename wallets_temp to wallets. This way, party_id will no longer be the primary key, and we'll be able to create wallets with an empty party_id.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my suggestion below to keep the party_id as the private key
| SELECT party_id, "primary", hint, public_key, "namespace", chain_id, signing_provider_id FROM wallets ON conflict do nothing | ||
| `.execute(db) | ||
|
|
||
| await db.schema.dropTable('wallets').ifExists().execute() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The migration down happens in reverse order. We transition from the new table with id as the primary key back to the old table with party_id as the primary key.
| * | ||
| */ | ||
| export interface Wallet { | ||
| id: WalletId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels unnecessary since partyId is unique...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to add an ID and start using it as the primary key because we're going to store the wallet in the database before we know its party_id. As a result, there may be multiple records with an empty party_id, so it will no longer be unique
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are able to form the partyId (hint::namespace, where namespace=hash(publicKey)) by using the public key you select in the controller here: (https://github.com/pixelplex/splice-wallet-kernel/blob/pixelplex/fireblocks-wallet-creation/wallet-gateway/remote/src/user-api/controller.ts#L224)
To create the fingerprint, you can use the createFingerprintFromKey method found here:
| static createFingerprintFromKey(publicKey: string): string |
As a result, we can predefine the partyId, before it's transactions are signed by fireblocks and the party is allocated on the participant node.
| }) | ||
|
|
||
| if (!['pending', 'signed'].includes(status)) { | ||
| await store.removeWallet(params.id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do we remove it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because if the status of our transaction is neither pending nor signed, it means it has either failed or been cancelled. In that case, we’ll never be able to allocate the wallet, so there’s no need to store it in the database
Signed-off-by: Alina Radyukevich <[email protected]>
| addWallet(wallet: Wallet): Promise<void> | ||
| // removeWallet(partyId: Wallet): Promise<void> | ||
| updateWallet(params: UpdateWallet): Promise<void> | ||
| removeWallet(partyId: string): Promise<void> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| removeWallet(partyId: string): Promise<void> | |
| removeWallet(partyId: PartyId): Promise<void> |
| setPrimaryWallet(partyId: PartyId): Promise<void> | ||
| addWallet(wallet: Wallet): Promise<void> | ||
| // removeWallet(partyId: Wallet): Promise<void> | ||
| updateWallet(params: UpdateWallet): Promise<void> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we just allow updating all fields of a wallet (identified by partyId):
updateWallet(wallet: Wallet): Promise<void>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it could be better just rename this method from updateWallet to updateWalletStatus?
#610