Skip to content
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

Adding Walletconnect endpoints for DIDs, Datalayer, and NFT Bulk Mint #2308

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Gaerax
Copy link
Contributor

@Gaerax Gaerax commented Mar 18, 2024

Added a walletconnect endpoints for almost all DID and Datalayer commands, and nft_mint_bulk and push_tx to enable bulk minting via WC, and creating feature rich dapps.

@Gaerax
Copy link
Contributor Author

Gaerax commented Mar 20, 2024

I also added an endpoint to allows an app to request permissions for a list commands. This would make for a much better user experience for dapps that require use of many commands, as the user can just be prompted once, at app launch to give permissions for all of the basic, read-only commands that it needs.

This includes a new confirm dialog, which could probably be improved, but it does the job.
image

If the app already has all of the permissions in the list, the command returns successful without prompting the user.

If the command list sent by the app contains a command that does not allow bypass, or is not a valid command name, it is ignored.

@Gaerax
Copy link
Contributor Author

Gaerax commented Mar 20, 2024

Since updating my repo with recent commits for the PR, I have found that some of the new WC commands which used to work fine, now have issues. I will switch this PR to a draft for the time being, until I figure out why that is.

@Gaerax Gaerax marked this pull request as draft March 20, 2024 05:28
@Gaerax Gaerax marked this pull request as ready for review March 25, 2024 05:25
@Gaerax
Copy link
Contributor Author

Gaerax commented Mar 25, 2024

There was no issue, I'm just a dumb dumb who forgot to run the build command

@ChiaMineJP
Copy link
Contributor

Thank you for the great PR!
Please give us time to review. We'll check the code.

Copy link
Contributor

@ChiaMineJP ChiaMineJP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just finished a part of reviewing. I'll do more check later.

'Offers',
'StoreMirrors',
'PendingRoots',
'Plugins',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'Files' is missing here

}),

addMissingFiles: mutation(build, DataLayer, 'addMissingFiles', {
invalidatesTags: (_result, _error, { ids }) => [{ type: 'Files', ids }],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to https://redux-toolkit.js.org/rtk-query/usage/automated-refetching#invalidating-tags, invalidateTags wouldn't recognize ids property here.
The type of invalidateTags is either {type: string; id?: string|number}, {type: string}, or just string.

So I think this line should be:
invalidateTags: {_result, _error, { ids }) => ids.map(id => ({type: 'Files', id}))

}),

cancelOffer: mutation(build, DataLayer, 'cancelOffer', {
invalidatesTags: (_result, _error, { tradeId }) => [{ type: 'Offers', tradeId }],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same as above. invalidateTags won't accept tradeId as a tag id. So this should be:

invalidatesTags: (_result, _error, { tradeId }) => [{ type: 'Offers', id: tradeId }],

}),

clearPendingRoots: mutation(build, DataLayer, 'clearPendingRoots', {
invalidatesTags: (_result, _error, { storeId }) => [{ type: 'PendingRoots', storeId }],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same as above.

@@ -142,6 +142,10 @@ export const fullNodeApi = apiWithTag.injectEndpoints({
getFeeEstimate: query(build, FullNode, 'getFeeEstimate', {
providesTags: [{ type: 'FeeEstimate' }],
}),

pushTx: mutation(build, FullNode, 'pushTx', {
invalidatesTags: [{ type: 'Transactions', id: 'LIST' }],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Transaction(singular) is better than Transactions (plural).

Need to add Transaction tag name to Line#10:
const apiWithTag = api.enhanceEndpoints({ addTagTypes: ['BlockchainState', 'FeeEstimate', 'FullNodeConnections'] });

@@ -167,6 +201,7 @@ export const {
useGetDIDNameQuery,
useSetDIDNameMutation,
useGetDIDRecoveryListQuery,
useUpdateDIDMetadataMutation,
useGetDIDInformationNeededForRecoveryQuery,
useGetDIDCurrentCoinInfoQuery,
useGetDIDInfoQuery,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • usePushTxMutation is missing in full node hooks.
  • useGetDIDMetadataQuery is missing in wallet hooks
  • useMintBulkMutation is missing in wallet hooks


updateDIDMetadata: mutation(build, DID, 'updateDidMetadata', {
invalidatesTags: (_result, _error, { walletId }) => [
{ type: 'DIDInfo', id: walletId },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type of id with tag DIDInfo should be coinId.
I don't think this's appropriate to be here.

// }),

getDIDMetadata: query(build, DID, 'getDidMetadata', {
providesTags: (result, _error, { walletId }) => (result ? [{ type: 'DIDMetadata', id: walletId }] : []),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tag name DIDMetadata should be added to line#10 tagTypes array.

{ type: 'DIDInfo', id: walletId },
{ type: 'DIDCoinInfo', id: walletId },
{ type: 'Wallets', id: walletId },
{ type: 'DIDWallet', id: walletId },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{ type: 'DIDMetadata', id: walletId} should be added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants