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

feat(spaceward): key creation in sidebar #728

Merged
merged 4 commits into from
Aug 30, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
fix key icon in sidebar
cleanup
  • Loading branch information
alex-nax committed Aug 28, 2024
commit be7e2d30ad4733ef6b06bda2a8c7487d71cc3506
2 changes: 1 addition & 1 deletion spaceward/pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

38 changes: 28 additions & 10 deletions spaceward/src/config/tokens.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { env } from "@/env";
import { getProvider } from "../lib/eth";

type ChainName = Parameters<typeof getProvider>[0];
Expand Down Expand Up @@ -222,17 +223,26 @@ Binance Coin native BNB
Ethereum Classic native ETC
Polygon native MATIC
*/
export const ENABLED_ETH_CHAINS: { chainName: ChainName; testnet?: boolean }[] =
[
{ chainName: "arbitrum" },
{ chainName: "base" },
{ chainName: "bsc" },
{ chainName: "mainnet" },
{ chainName: "optimism" },
{ chainName: "sepolia", testnet: true },
];
const _ENABLED_ETH_CHAINS: { chainName: ChainName; testnet?: boolean }[] = [
{ chainName: "arbitrum" },
{ chainName: "base" },
{ chainName: "bsc" },
{ chainName: "mainnet" },
{ chainName: "optimism" },
{ chainName: "sepolia", testnet: true },
];

export const ENABLED_ETH_CHAINS = _ENABLED_ETH_CHAINS.filter(({ testnet }) =>
env.networkVisibility === "all"
? true
: env.networkVisibility === "mainnet"
? !testnet
: Boolean(testnet),
);
Comment on lines +226 to +241
Copy link
Contributor

Choose a reason for hiding this comment

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

Approve the filtering logic for Ethereum chains.

The implementation of the filtering logic based on the networkVisibility environment variable is sound. It enhances the flexibility and security by ensuring that only the appropriate chains are exposed based on the operational environment.

The changes are approved.

It would be beneficial to add unit tests to verify the filtering logic under different networkVisibility settings. Would you like assistance in setting up these tests?


export const COSMOS_CHAINS: {
console.log("ENABLED_ETH_CHAINS", ENABLED_ETH_CHAINS);
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider conditional logging for production environments.

The addition of a console log to output ENABLED_ETH_CHAINS can be useful for debugging but might not be suitable for a production environment due to potential clutter and information leakage.

Consider wrapping this log statement with a condition to check if the environment is not production:

+ if (env.nodeEnv !== 'production') {
+   console.log("ENABLED_ETH_CHAINS", ENABLED_ETH_CHAINS);
+ }
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
console.log("ENABLED_ETH_CHAINS", ENABLED_ETH_CHAINS);
if (env.nodeEnv !== 'production') {
console.log("ENABLED_ETH_CHAINS", ENABLED_ETH_CHAINS);
}


const _COSMOS_CHAINS: {
chainName: string;
feeAmount?: string;
rpc?: string;
Expand Down Expand Up @@ -272,3 +282,11 @@ export const COSMOS_CHAINS: {
testnet: true,
},
];

export const COSMOS_CHAINS = _COSMOS_CHAINS.filter(({ testnet }) =>
env.networkVisibility === "all"
? true
: env.networkVisibility === "mainnet"
? !testnet
: Boolean(testnet),
);
3 changes: 3 additions & 0 deletions spaceward/src/env.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ const aminoAnalyzerContract =
const p2pRelayURL =
import.meta.env.VITE_P2P_RELAY_URL ||
"https://relay.devnet.wardenprotocol.org:443";
const networkVisibility: "testnet" | "mainnet" | "all" =
import.meta.env.VITE_WARDEN_NETWORK_VISIBILITY || "testnet";

export const env = {
apiURL,
Expand All @@ -38,4 +40,5 @@ export const env = {
ethereumAnalyzerContract,
aminoAnalyzerContract,
p2pRelayURL,
networkVisibility,
};
153 changes: 55 additions & 98 deletions spaceward/src/features/actions/Sidebar.tsx
Original file line number Diff line number Diff line change
@@ -1,49 +1,53 @@
import clsx from "clsx";
import { useEffect } from "react";
import { useChain } from "@cosmos-kit/react-lite";
import { env } from "@/env";
import { QueuedAction, QueuedActionStatus, useActionsState } from "./hooks";
import { getClient, getSigningClient } from "@/hooks/useClient";
import { cosmos, warden } from "@wardenprotocol/wardenjs";
import { hexlify, Transaction } from "ethers";
import { useContext, useEffect } from "react";
import { isDeliverTxSuccess, StargateClient } from "@cosmjs/stargate";
import { useChain, walletContext } from "@cosmos-kit/react-lite";
import { KeyringSnapRpcClient } from "@metamask/keyring-api";
import { cosmos, warden } from "@wardenprotocol/wardenjs";
import { base64FromBytes } from "@wardenprotocol/wardenjs/codegen/helpers";
import { ActionStatus } from "@wardenprotocol/wardenjs/codegen/warden/act/v1beta1/action";
import { hexlify, Transaction } from "ethers";
import { getProvider, isSupportedNetwork } from "@/lib/eth";
import { useToast } from "@/components/ui/use-toast";

import {
Popover,
PopoverContent,
PopoverTrigger,
} from "@/components/ui/popover";
import "./animate.css";
import { isUint8Array } from "@/lib/utils";
import { prepareTx } from "../modals/util";

import { COSMOS_CHAINS } from "@/config/tokens";
import { env } from "@/env";
import { getClient, getSigningClient } from "@/hooks/useClient";
import { useWeb3Wallet } from "@/hooks/useWeb3Wallet";
import { base64FromBytes } from "@wardenprotocol/wardenjs/codegen/helpers";
import { useToast } from "@/components/ui/use-toast";
import { KeyringSnapRpcClient } from "@metamask/keyring-api";
import { getProvider, isSupportedNetwork } from "@/lib/eth";
import { isUint8Array } from "@/lib/utils";
import "./animate.css";
import { QueuedAction, QueuedActionStatus, useActionsState } from "./hooks";
import { getActionHandler } from "./util";
import { prepareTx } from "../modals/util";
import { TEMP_KEY, useKeySettingsState } from "../keys/state";
import Assets from "../keys/assets";

interface ItemProps extends QueuedAction {
single?: boolean;
}

type GetStatus = (client: Awaited<ReturnType<typeof getClient>>) => Promise<{
pending: boolean;
error: boolean;
done: boolean;
next?: "eth" | "eth-raw" | "cosmos";
value?: any;
}>;

function ActionItem({ single, ...item }: ItemProps) {
const { toast } = useToast();
const { walletManager } = useContext(walletContext);
const { data: ks, setData: setKeySettings } = useKeySettingsState();
const { toast } = useToast()
const { w } = useWeb3Wallet("wss://relay.walletconnect.org");
const { setData } = useActionsState();

const { getOfflineSignerDirect: getOfflineSigner } = useChain(
env.cosmoskitChainName,
);

const type = typeof item.keyThemeIndex !== "undefined" ?
"key" : (["walletConnectRequestId", "walletConnectTopic"] as const).every(key => typeof item[key] !== "undefined") ?
"wc" : typeof item.snapRequestId ?
"snap" : undefined

Comment on lines +36 to +50
Copy link
Contributor

Choose a reason for hiding this comment

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

Refactor action type determination logic for clarity and maintainability.

The new implementation for determining the action type (type) is more concise and readable. However, there's a static analysis error reported on line 48 due to a constant condition in the ternary operation. This might be a false positive, but it's worth verifying if the logic can be simplified or if there's an actual issue with the condition always evaluating to a constant value.

Consider revising the logic to ensure it's dynamic and correctly handles various conditions without resulting in constant outcomes. Here's a proposed refactor to address the potential issue:

- const type = typeof item.keyThemeIndex !== "undefined" ?
-     "key" : (["walletConnectRequestId", "walletConnectTopic"] as const).every(key => typeof item[key] !== "undefined") ?
-         "wc" : typeof item.snapRequestId ?
-             "snap" : undefined
+ const determineType = () => {
+     if (typeof item.keyThemeIndex !== "undefined") return "key";
+     if (item.walletConnectRequestId && item.walletConnectTopic) return "wc";
+     if (item.snapRequestId) return "snap";
+     return undefined;
+ };
+ const type = determineType();

This refactoring ensures that each condition is evaluated independently and only when necessary, potentially resolving the static analysis error.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const { walletManager } = useContext(walletContext);
const { data: ks, setData: setKeySettings } = useKeySettingsState();
const { toast } = useToast()
const { w } = useWeb3Wallet("wss://relay.walletconnect.org");
const { setData } = useActionsState();
const { getOfflineSignerDirect: getOfflineSigner } = useChain(
env.cosmoskitChainName,
);
const type = typeof item.keyThemeIndex !== "undefined" ?
"key" : (["walletConnectRequestId", "walletConnectTopic"] as const).every(key => typeof item[key] !== "undefined") ?
"wc" : typeof item.snapRequestId ?
"snap" : undefined
const { walletManager } = useContext(walletContext);
const { data: ks, setData: setKeySettings } = useKeySettingsState();
const { toast } = useToast()
const { w } = useWeb3Wallet("wss://relay.walletconnect.org");
const { setData } = useActionsState();
const { getOfflineSignerDirect: getOfflineSigner } = useChain(
env.cosmoskitChainName,
);
const determineType = () => {
if (typeof item.keyThemeIndex !== "undefined") return "key";
if (item.walletConnectRequestId && item.walletConnectTopic) return "wc";
if (item.snapRequestId) return "snap";
return undefined;
};
const type = determineType();
Tools
Biome

[error] 48-48: Unexpected constant condition.

(lint/correctness/noConstantCondition)

Comment on lines +46 to +50
Copy link
Contributor

Choose a reason for hiding this comment

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

Refactor action type determination logic to address static analysis error.

The refactored logic for determining the action type is more concise and readable. However, there's a static analysis error due to a constant condition in the ternary operation. Consider revising the logic to ensure it handles various conditions correctly.

- const type = typeof item.keyThemeIndex !== "undefined" ?
-     "key" : (["walletConnectRequestId", "walletConnectTopic"] as const).every(key => typeof item[key] !== "undefined") ?
-         "wc" : typeof item.snapRequestId ?
-             "snap" : undefined
+ const determineType = () => {
+     if (typeof item.keyThemeIndex !== "undefined") return "key";
+     if (item.walletConnectRequestId && item.walletConnectTopic) return "wc";
+     if (item.snapRequestId) return "snap";
+     return undefined;
+ };
+ const type = determineType();
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const type = typeof item.keyThemeIndex !== "undefined" ?
"key" : (["walletConnectRequestId", "walletConnectTopic"] as const).every(key => typeof item[key] !== "undefined") ?
"wc" : typeof item.snapRequestId ?
"snap" : undefined
const determineType = () => {
if (typeof item.keyThemeIndex !== "undefined") return "key";
if (item.walletConnectRequestId && item.walletConnectTopic) return "wc";
if (item.snapRequestId) return "snap";
return undefined;
};
const type = determineType();
Tools
Biome

[error] 48-48: Unexpected constant condition.

(lint/correctness/noConstantCondition)

useEffect(() => {
if (item.status === QueuedActionStatus.Signed) {
const signer = getOfflineSigner();
Expand Down Expand Up @@ -174,6 +178,7 @@ function ActionItem({ single, ...item }: ItemProps) {
) as unknown as number;
} else {
console.error("action failed", res);

toast({
title: "Failed",
description: "Unexpected action status",
Expand All @@ -198,73 +203,7 @@ function ActionItem({ single, ...item }: ItemProps) {
} else if (item.status === QueuedActionStatus.ActionReady) {
let cancel = false;
let timeout: number | undefined;
let getStatus: GetStatus | undefined;

switch (item.action?.result?.typeUrl) {
case warden.warden.v1beta3.MsgNewSignRequestResponse.typeUrl: {
const { id } =
warden.warden.v1beta3.MsgNewSignRequestResponse.decode(
item.action.result.value,
);

getStatus = (client) =>
client.warden.warden.v1beta3
.signRequestById({ id })
.then(({ signRequest }) => {
switch (signRequest?.status) {
case warden.warden.v1beta3.SignRequestStatus
.SIGN_REQUEST_STATUS_PENDING:
return {
pending: true,
error: false,
done: false,
};
case warden.warden.v1beta3.SignRequestStatus
.SIGN_REQUEST_STATUS_FULFILLED:
const analyzers = (
item.data as Parameters<
typeof warden.warden.v1beta3.MsgNewSignRequest.encode
>[0]
).analyzers;
const {
walletConnectRequestId,
walletConnectTopic,
snapRequestId
} = item;

return {
pending: false,
error: false,
done: true,
next: analyzers.includes(
env.ethereumAnalyzerContract,
)
? "eth"
: analyzers.includes(
env.aminoAnalyzerContract,
)
? "cosmos"
: // fixme
(walletConnectRequestId &&
walletConnectTopic) || snapRequestId
? "eth-raw"
: undefined,
value: signRequest?.signedData,
};
default:
return {
pending: false,
error: true,
done: true,
};
}
});

break;
}
default:
console.warn("unknown action", item.action);
}
const { getStatus } = getActionHandler(item);

async function checkResult() {
if (cancel || !getStatus) {
Expand All @@ -273,7 +212,7 @@ function ActionItem({ single, ...item }: ItemProps) {

const client = await getClient();
const status = await getStatus(client);

// TMP_KEY to key Id
if (status.error) {
toast({
title: "Failed",
Expand All @@ -296,6 +235,14 @@ function ActionItem({ single, ...item }: ItemProps) {
duration: 10000,
});
}

if (type === "key" && typeof status.value === "bigint") {
const keyId = status.value;
const settings = { ...ks?.settings, [keyId.toString()]: ks?.settings?.[TEMP_KEY] };
delete settings[TEMP_KEY];
setKeySettings({ settings })
}
Comment on lines +239 to +244
Copy link
Contributor

Choose a reason for hiding this comment

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

Enhance state management logic for key settings updates.

The logic for updating key settings based on the action result is a critical part of the component's functionality. The implementation correctly checks for the action type and status before updating the settings. However, there's a potential issue with direct manipulation of state objects which might lead to unintended side effects.

Ensure that state updates are handled immutably to prevent issues with React's state management. Here's a proposed change to handle state updates more safely:

- const settings = { ...ks?.settings, [keyId.toString()]: ks?.settings?.[TEMP_KEY] };
- delete settings[TEMP_KEY];
- setKeySettings({ settings })
+ const updatedSettings = { ...ks?.settings };
+ if (TEMP_KEY in updatedSettings) {
+     updatedSettings[keyId.toString()] = updatedSettings[TEMP_KEY];
+     delete updatedSettings[TEMP_KEY];
+ }
+ setKeySettings({ settings: updatedSettings });

This change ensures that the settings object is copied and modified as a new instance, preserving immutability and potentially preventing bugs related to state updates.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (type === "key" && typeof status.value === "bigint") {
const keyId = status.value;
const settings = { ...ks?.settings, [keyId.toString()]: ks?.settings?.[TEMP_KEY] };
delete settings[TEMP_KEY];
setKeySettings({ settings })
}
if (type === "key" && typeof status.value === "bigint") {
const keyId = status.value;
const updatedSettings = { ...ks?.settings };
if (TEMP_KEY in updatedSettings) {
updatedSettings[keyId.toString()] = updatedSettings[TEMP_KEY];
delete updatedSettings[TEMP_KEY];
}
setKeySettings({ settings: updatedSettings });
}


setData({
[item.id]: status.next
? {
Expand Down Expand Up @@ -335,8 +282,6 @@ function ActionItem({ single, ...item }: ItemProps) {
return;
}

const type = snapRequestId ? "snap" : (walletConnectRequestId && walletConnectTopic) ? "wc" : undefined

switch (type) {
case "wc": {
if (!w) {
Expand Down Expand Up @@ -426,7 +371,7 @@ function ActionItem({ single, ...item }: ItemProps) {
}).then(() => true);
}
} else {
const provider = getProvider(chainName);
const provider = getProvider(chainName).provider;

promise = provider
.broadcastTransaction(signedTx.serialized)
Expand Down Expand Up @@ -541,13 +486,22 @@ function ActionItem({ single, ...item }: ItemProps) {
signatures: [sig],
});

promise = StargateClient.connect(chain.rpc)
let getRpc: Promise<string>;

if (chain.rpc) {
getRpc = Promise.resolve(chain.rpc);
} else {
const repo = walletManager.getWalletRepo(chainName);
repo.activate();
getRpc = repo.getRpcEndpoint().then(endpoint => endpoint ? typeof endpoint === "string" ? endpoint : endpoint.url : "https://rpc.cosmos.directory/" + chainName);
}

promise = getRpc.then(rpc => StargateClient.connect(rpc))
.then((client) => {
return client.broadcastTx(
cosmos.tx.v1beta1.TxRaw.encode(
txRaw,
).finish(),
// fixme
);
})
.then((res) => {
Expand Down Expand Up @@ -627,7 +581,10 @@ function ActionItem({ single, ...item }: ItemProps) {
"mx-2 p-3 rounded-lg": !single,
})}
>
<p className="text-lg font-semibold">Action</p>
<div className="flex items-center">
{type === "key" ? <Assets.themeSelector className="w-8 h-5 mr-2" themeIndex={item.keyThemeIndex ?? 0} /> : null}
<p className="text-lg font-semibold">{item.title ?? "Action"}</p>
</div>

<p className="text-sm text-gray-500 mb-2">
{item.status === QueuedActionStatus.Signed
Expand Down Expand Up @@ -665,7 +622,7 @@ function ActionItem({ single, ...item }: ItemProps) {
/>
</div>
</div>
</div>
</div >
);
}

Expand Down
9 changes: 8 additions & 1 deletion spaceward/src/features/actions/hooks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@ export interface QueuedAction {
/** @deprecated fix naming */
signDoc?: StdSignDoc;
pubkey?: Uint8Array;

title?: string;
keyThemeIndex?: number;
walletConnectRequestId?: number;
walletConnectTopic?: string;
snapRequestId?: string;
Expand All @@ -73,6 +74,8 @@ export function useEnqueueAction<Data>(
tx?: TransactionLike;
signDoc?: StdSignDoc;
pubkey?: Uint8Array;
title?: string;
keyThemeIndex?: number;
walletConnectRequestId?: number;
walletConnectTopic?: string;
snapRequestId?: string;
Expand All @@ -84,7 +87,9 @@ export function useEnqueueAction<Data>(
tx,
pubkey,
signDoc,
title,
snapRequestId,
keyThemeIndex,
walletConnectRequestId,
walletConnectTopic,
...opts
Expand All @@ -111,8 +116,10 @@ export function useEnqueueAction<Data>(
chainName,
pubkey,
signDoc,
title,
snapRequestId,
tx,
keyThemeIndex,
walletConnectRequestId,
walletConnectTopic,
},
Expand Down
Loading
Loading