-
Notifications
You must be signed in to change notification settings - Fork 5
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: add psbt functionality #641
base: dev
Are you sure you want to change the base?
Conversation
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.
Thanks @edgarkhanzadian, just dropping some comments in here. Main thoughts is that I think it isn't necessary to pass around addresses the way being done here.
apps/mobile/src/store/keychains/bitcoin/bitcoin-keychains.read.ts
Outdated
Show resolved
Hide resolved
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.
Great work @edgarkhanzadian this is coming together 👌🏼
apps/mobile/src/features/approver/components/inputs-outputs-card.tsx
Outdated
Show resolved
Hide resolved
<key>ITSAppUsesNonExemptEncryption</key> | ||
<false /> |
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.
Fixing the appstore connect deployment, s.t. appstore connect does not ask for manual approval every time we upload a build to testflight
@@ -28,7 +28,7 @@ const accountUtxoResponseSchema = z.array(utxoSchema); | |||
export type Utxo = z.infer<typeof utxoSchema>; | |||
|
|||
export function createUtxoAccountCacheKey(network: string, descriptor: string) { | |||
return ['btc-utxos', network, descriptor.substring(0, 10)]; | |||
return ['btc-utxos', network, descriptor.substring(0, 35)]; |
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.
10 characters were not enough to make a unique xpub key for multiple accounts, extending it to 35 fixed it
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.
yep I saw a similar bug elsewhere
@@ -78,9 +79,13 @@ export function createMarketDataService( | |||
.map(r => r.value) | |||
.filter(isDefined); | |||
if (prices.length === 0) throw new Error('Unable to fetch price data: ' + currency); | |||
|
|||
const meanPrice = calculateMeanAverage(prices); | |||
const meanPriceFractional = convertAmountToFractionalUnit(meanPrice, currencyDecimalsMap.USD); |
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.
Services should return cents, not dollars
} | ||
|
||
function selectByAccountIds(accountIds: string[]) { | ||
return createSelector(selectors.selectEntities, entities => { |
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.
selectors.selectAll
does the mapping you do 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.
selectAll returns an array of accounts which i'd need to filter up using accountIds array. In this case, entities is a map from which i can select only the accounts that i need, without going through all accounts accountIds.length
times. Thought to include a small optimization for big wallets
const btcBalance = createMoney(Number(inputOutput.value), 'BTC'); | ||
const usdBalance = btcMarketData | ||
? baseCurrencyAmountInQuote(btcBalance, btcMarketData) | ||
: createMoney(0, 'USD'); |
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've seen this used in few other places, and it can potentially cause flicker from 0 to actual balance, unless the relevant query happens to be in cache and fresh. Is there anything we're missing design-wise to avoid this?
Couple things I can think of:
- Loading state (if applicable)
- Making shared data like market queries globally available upon app startup
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.
Agree, we need to think of that but we might need to handle it as a part of a separate PR
apps/mobile/src/features/send/send-form/hooks/use-send-form-btc.tsx
Outdated
Show resolved
Hide resolved
|
||
export function FeeCard({ feeType }: { feeType: FeeTypes }) { | ||
export function FeeCard({ feeType, amount }: { feeType: FeeTypes; amount: Money }) { |
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 use a props interface?
|
||
export function OutcomesCard() { |
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.
What does OutcomesCard
refer to? Not sure I understand what this is exactly?
// eslint-disable-next-line no-console, lingui/no-unlocalized-strings | ||
console.log('Send form data:', values); | ||
async onInitSendTransfer(values: SendFormBtcSchema) { | ||
if (!sortedUtxos[0]) throw new Error('no utxo'); |
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.
Was this all just for testing? Please don't merge over my work in latest btc send form PR to gen the unsigned psbt.
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.
Yeap! i created a very basic psbt here just for testing purposes until your changes get merged.
Great work @edgarkhanzadian, can you rebase on my latest merge before I approve? My changes in onInitSendTransfer in use-send-form.tsx should generate the unsigned psbt to pass to your work.
Sounds awesome! Will update the PR
Great work @edgarkhanzadian, can you rebase on my latest merge before I approve? My changes in |
Note: I'll leave the commits unsquashed for now until right before the merge, just in case i need to revert some of the changes.