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

[Canary] Alpha release #936

Closed
wants to merge 76 commits into from
Closed

[Canary] Alpha release #936

wants to merge 76 commits into from

Conversation

hstove
Copy link
Contributor

@hstove hstove commented Jan 29, 2021

Try out this version of the Stacks Wallet - download extension builds or the Stacks testnet demo app

To decrease the confusion around having multiple open PRs for the Stacks Wallet for Web release, this single PR can be treated as the official "canary" release branch.

NPM tags will be set to "canary" for Connect packages. Builds from this PR will be deployed to the web wallet install page.

name: connect-extension
path: connect-extension.zip
name: stacks-wallet-chromium
path: stacks-wallet-chromium.zip
Copy link
Collaborator

Choose a reason for hiding this comment

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

version number in filename?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is then uploaded to GCS, which is how it's downloaded from hiro.so. We'd have to duplicate the .zip and then upload the version without a filename.

There is more work required to update our actual production flows, because right now it's quite tied to the app.blockstack.org production flow. I'm going to table this, it's a good suggestion, but I'd rather combine it with other CI work.

import { fetchPendingTxs } from '@common/api/transactions';
import { fetchFromSidecar } from '@common/api/fetch';

export const fetchBalances = (apiServer: string) => (
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could also use the generated client, I'm hybrid-using it in a wrapper

@@ -2,3 +2,6 @@ declare const EXT_ENV: string;
declare const NODE_ENV: string;
declare const SEGMENT_KEY: string;
declare const STATS_URL: string;
declare const COMMIT_SHA: string;
declare const VERSION: string;
declare const BRANCH: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Personal pref would be to have all these in a single global object, just for dev x of seeing them in the autocomplete

@@ -1,28 +1,26 @@
import { useEffect, useCallback } from 'react';
import { useSelector } from 'react-redux';
import { selectAuthRequest } from '@store/onboarding/selectors';
// import { useSelector } from 'react-redux';
Copy link
Collaborator

Choose a reason for hiding this comment

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

kill

Comment on lines 140 to 156
const newConditions: PostCondition[] = payload.postConditions.map(postCondition => {
let newCondition: PostCondition;
if (typeof postCondition === 'string') {
const reader = BufferReader.fromBuffer(Buffer.from(postCondition, 'hex'));
newCondition = deserializePostCondition(reader);
} else {
newCondition = { ...postCondition };
}
// override principal with current user
newCondition = {
...newCondition,
principal: parsePrincipalString(currentAddress),
};
return newCondition;
});
set(postConditionsStore, newConditions);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit pick, but this is kinda mixing programming styles imo. Mapping an array, functional, then reassigning a ref, imperative. No action needed, just an observation that it'd look cleaner with a const lol.

{currentNetworkKey === 'mainnet' ? (
<Text
as="a"
href="https://coinmarketcap.com/currencies/blockstack/markets/"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've been putting all external urls in a constants file

Comment on lines 43 to 46
const fungibleTokens = Object.keys(balances.fungible_tokens).map(key => {
const token = balances.fungible_tokens[key];
const friendlyName = key.split('::')[1];
return (
Copy link
Collaborator

Choose a reason for hiding this comment

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

Think I've seen logic like this earlier, util fn?

friendlyName={friendlyName}
key={key}
value={token.balance}
subtitle={friendlyName.slice(0, 3).toUpperCase()}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Definitely seen this slice before, also a util fn here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ultimately all of this is a smell that we need to remove at the design level. It needs to be data that's pulled from the token itself, or simply not have a "ticker" at all. These values don't mean anything


const getTxLabel = (tx: Tx): string => {
if (tx.tx_type === 'token_transfer') return 'Stacks Transfer';
if (tx.tx_status === 'pending') return 'Pending Transaction';
Copy link
Collaborator

Choose a reason for hiding this comment

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

if you have a pending tx token transfer it won't ever get here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a very good catch


type Tx = MempoolTransaction | Transaction;

const getAssetTransfer = (tx: Tx): TransactionEventFungibleAsset | undefined => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Mixing between error case returning null or undefined. I'm a null man, myself. I've seen others advocating sticking to undefined. Either way, consistency 👍🏼

@kyranjamie
Copy link
Collaborator

80% style/convention/preference comments, and would encourage for an interactive rebase to squash some of unneeded commits.

Otherwise it's looking pretty good. I'd advocate making more separate layout/styling components. When all the logic & styling is in the same component, core functionality gets lots in amongst the styling props etc.

name: connect-extension
path: connect-extension.zip
name: stacks-wallet-chromium
path: stacks-wallet-chromium.zip
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Comment on lines +19 to +31
'@typescript-eslint/no-unsafe-assignment': [0],
'@typescript-eslint/no-unsafe-return': [0],
'@typescript-eslint/no-unsafe-call': [0],
'@typescript-eslint/no-unsafe-member-access': [0],
'@typescript-eslint/ban-types': [0],
'@typescript-eslint/restrict-template-expressions': [0],
'@typescript-eslint/explicit-module-boundary-types': [0],
"no-warning-comments": [1],
"react-hooks/exhaustive-deps": [
"warn", {
additionalHooks: "useRecoilCallback",
},
]
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add these to @stacks/eslint-config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, I'm not sure. Some you could argue are better left enabled, and anything React should be an extended config, or something.

import { useLoadable } from './use-loadable';

export const useFetchAccountData = () => {
const accountData = useLoadable(accountDataStore);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const accountData = useLoadable(accountDataStore);
return useLoadable(accountDataStore);

};

export const useFetchBalances = () => {
const accountBalances = useLoadable(accountBalancesStore);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const accountBalances = useLoadable(accountBalancesStore);
return useLoadable(accountBalancesStore);

document.documentElement.classList.add('dark');
document.documentElement.classList.remove('light');
setHtmlBackgroundColor();
}, []);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
}, []);
}, [setHtmlBackgroundColor]);

return (
<Flex alignItems="center" maxWidth="100%" {...rest}>
<Flex flex={1} maxWidth="100%">
<Flex flex={1} maxWidth="100%" dir="column">
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<Flex flex={1} maxWidth="100%" dir="column">
<Flex flex={1} maxWidth="100%" flexDirection="column">

});
const { color, bg, borderColor, placeholderColor } = defaultConfig(theme).light;

const cssReset = css`
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah we should be able to use the one from @stacks/ui

Comment on lines 29 to 44
const TabHeader: React.FC<{ tab: Tab }> = ({ tab }) => {
const tabName = new String(tab);
return (
<Box
width="50%"
textAlign="center"
p="base-tight"
cursor="pointer"
fontSize="14px"
{...getTabStyles(tab)}
onClick={() => setCurrentTab(tab)}
>
<Text fontSize={2}>{tabName.charAt(0).toUpperCase() + tabName.slice(1)}</Text>
</Box>
);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

ideally not

Comment on lines 120 to 132
const Settings: React.FC<BoxProps> = props => {
if (hideActions) return null;
return (
<IconButton
size="42px"
iconSize="24px"
onClick={() => setShowSettings(true)}
color="black"
icon={IconMenu2}
{...props}
/>
);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

should take this out into its own. v bad perf

</Box>
);

export const SettingsPopover: React.FC = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

@hstove hstove linked an issue Mar 5, 2021 that may be closed by this pull request
7 tasks
@hstove
Copy link
Contributor Author

hstove commented Mar 8, 2021

Woohoo! We can close this, since the code in this branch has been pushed directly to the main branch, which is our new default branch.

@hstove hstove closed this Mar 8, 2021
@kyranjamie kyranjamie deleted the release/canary branch June 14, 2021 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modify CI and branches for Web Wallet alpha release
5 participants