Paul/saved action #574

paullinator · 2023-11-30T03:51:07Z

CHANGELOG

Does this branch warrant an entry to the CHANGELOG?

Yes
No

Dependencies

none

Description

none

To see the specific tasks where the Asana app for GitHub is being used, see below:
- https://app.asana.com/0/0/1206044971437796

src/core/currency/wallet/currency-wallet-api.ts

swansontec · 2023-12-01T21:33:53Z

src/core/currency/wallet/currency-wallet-reducer.ts

-
-  nativeAmount: { [currencyCode: string]: string }
-  networkFee: { [currencyCode: string]: string }
+  tokenId: string | null


There cannot be a tokenId inside a MergedTransaction, because this combines all the coins together.

I see that there is a currencyCode in there already. This was a mistake from Eliran, and we should delete it.

The only place where we use the tokenId / currencyCode is in the onBlockHeightChanged processor. Since the goal is to update the block height, we either need to iterate over Object.keys(MergedTransaction.tokens) to update every currency, or since that will cost a lot of bridge traffic, we could just use the default currency code / null tokenId, but fix the GUI to handle block-height changes in a currency-agnostic way.

Ok. Agreed. But such a change is a bit outside the scope of this PR. Can we punt that to another task. The tokenId here at least mirrors the currencyCode use and they can both be fixed simultaneously when we agree on the method.

swansontec · 2023-12-01T21:52:39Z

src/core/currency/wallet/currency-wallet-cleaners.ts

+export const asNumeralString = (raw: any): string => {
+  if (typeof raw !== 'string') throw new Error('Not type string')
+  if (isNaN(Number(raw))) throw new Error('Does not parse as a number')
+  return raw
+}


We already have asIntegerString. Please use that.

Otherwise, if you want decimal support, we should make an asDecimalString using a regexp. Remember, biggystring doesn't support 1E+6 syntax, but JavaScript does, so we need to be more picky than just using isNaN(Number(raw)). Inputs could pass this filter that still crash the app.

The intent was decimal support although unneeded for its current use. Once Sam merges his biggystring PR we'll have full scientific notation support. For now I'll change to asIntegerString

Thanks! I think this is the right solution in any case, since these are native amounts. Native amounts should never have decimal points or exponents.

src/core/currency/wallet/currency-wallet-api.ts

test/core/currency/wallet/currency-wallet.test.ts

src/core/currency/wallet/currency-wallet-cleaners.ts

swansontec · 2023-12-01T22:37:19Z

src/core/currency/wallet/currency-wallet-cleaners.ts

+export const asEdgeTxActionFiatType: Cleaner<EdgeTxActionFiatType> = asValue(
+  'buy',
+  'sell',
+  'sellNetworkFee'


OK, so with this design, we need to save two copies of the action, duplicating the orderId and all other fields. One copy goes with the parent currency as a fee, and one goes with the token. The only difference between the two copies is the type field.

You already have a tokenId inside the action itself, so there is no need for this duplication. The GUI can easily see which side is the fee and which side is the sell by using tx.tokenId === savedAction.cryptoAsset.tokenId.

So, I really think the whole concept of saving the action per-currency is a mistake. There should be one action per transaction, saved at the root level.

When we first discussed this, I trusted your assertion that the actions needed to be saved per-token. Now that I'm seeing what the action looks like, I think that's a deeply flawed assumption.

In the case of a fiat transaction this might be simple, but consider a DEX swap on Ripple. You have an action to create an order which pays XRP, the outgoing send which is considered a 'swap' which could pay USD.bs, then the receive which is the fulfillment that receives USD.gh. Do you really want the GUI to have to figure that all out.

As well the plugins report transactions per asset as they send individual EdgeTransactions. Now while they might not use savedAction but instead action, they would still be putting the action info per asset which we don't want to conflict.

swansontec

Just some changes on the metadata stuff.

I don't really understand the new savedAction / chainAction / assetAction design. I see that savedAction / assetAction are per-transaction, which makes sense, and the new assetAction is per asset. What I don't understand is how the plugin / app coordinate on setting up assetAction.

swansontec · 2023-12-18T18:28:43Z

src/core/currency/wallet/currency-wallet-api.ts

+export const PARENT_TOKEN_ID = ''
+


I wonder if this line should live in currency-wallet-files.ts, since it's related to the on-disk representation.

swansontec · 2023-12-18T18:49:58Z

src/core/currency/wallet/currency-wallet-cleaners.ts

+  category?: string | null
  exchangeAmount: { [fiatCurrencyCode: string]: number }
-  name?: string
-  notes?: string
+  name?: string | null
+  notes?: string | null


We don't actually want to store null on disk. The only reason we allow null in the user-facing type is to be able to delete fields. We need to revert this type definition, plus whatever changes are needed to the merging / updating code to make that work.

If could make sense to defined an EdgeMetadataChange type that includes | null for the fields that can be deleted, and revert the original EdgeMetadata type to how it was before. That makes it clear that metadata will never contain nulls. It's just a sigil for deleting fields.

swansontec · 2023-12-18T18:50:38Z

src/core/currency/wallet/currency-wallet-cleaners.ts

+  category: asNullable(asString),
  exchangeAmount: asOptional(asObject(asNumber), () => ({})),
-  name: asOptional(asString),
-  notes: asOptional(asString)
+  name: asNullable(asString),
+  notes: asNullable(asString)


Same here. We don't want to save null on disk.

paullinator · 2023-12-19T05:51:57Z

Just some changes on the metadata stuff.

I don't really understand the new savedAction / chainAction / assetAction design. I see that savedAction / assetAction are per-transaction, which makes sense, and the new assetAction is per asset. What I don't understand is how the plugin / app coordinate on setting up assetAction.

This works as follows. Plugins edit chainAction which is per tx and chainAssetAction which is per asset per tx

GUI edits savedAction which is per tx and assetAction which is per asset per tx. All four are needed unfortunately because of separation of chain vs gui and per asset vs per tx.

We need to provide both per-asset and per-transaction information, both on-disk and from the plugin.

This allows us to replace SwapData with EdgeTxActionSwap

swansontec requested changes Dec 1, 2023

View reviewed changes

paullinator force-pushed the paul/savedAction branch 7 times, most recently from d25d4f7 to 1cb4bba Compare December 6, 2023 23:57

paullinator force-pushed the paul/savedAction branch from 1cb4bba to 3ed9d40 Compare December 12, 2023 04:13

swansontec requested changes Dec 18, 2023

View reviewed changes

paullinator force-pushed the paul/savedAction branch 2 times, most recently from d1a4196 to fe7e855 Compare December 19, 2023 05:15

paullinator force-pushed the paul/savedAction branch from fe7e855 to bd5851f Compare December 22, 2023 02:03

swansontec force-pushed the paul/savedAction branch 2 times, most recently from 37f4e76 to ebb6513 Compare January 4, 2024 01:55

swansontec mentioned this pull request Jan 4, 2024

core2.0 - Require EdgeTokenId instead of currencyCode throughout API #577

Merged

2 tasks

swansontec approved these changes Jan 4, 2024

View reviewed changes

paullinator force-pushed the paul/savedAction branch from ebb6513 to 7fce6f5 Compare January 4, 2024 18:38

paullinator and others added 8 commits January 4, 2024 11:44

Rename action to chainAction

276cf75

Add other asset types to EdgeTransaction

584d5fb

We need to provide both per-asset and per-transaction information, both on-disk and from the plugin.

Add EdgeCurrencyWallet.saveTxAction

0da627c

Add EdgeTxActionFiat tx action

25a79ee

Extend EdgeTxActionSwap params

f23d161

This allows us to replace SwapData with EdgeTxActionSwap

Rename source/destAsset to from/toAsset

ed4998e

Add transfer and tokenApproval types

c0a0817

Add stakeNetworkFee and unstakeNetworkFee

427c168

paullinator force-pushed the paul/savedAction branch from 7fce6f5 to 427c168 Compare January 4, 2024 19:55

paullinator enabled auto-merge January 4, 2024 19:59

paullinator merged commit 1127c3c into master Jan 4, 2024
3 checks passed

paullinator deleted the paul/savedAction branch January 4, 2024 20:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Paul/saved action #574

Paul/saved action #574

paullinator commented Nov 30, 2023 •

edited

Loading

swansontec Dec 1, 2023

paullinator Dec 2, 2023

swansontec Dec 1, 2023

paullinator Dec 1, 2023

swansontec Dec 4, 2023 •

edited

Loading

swansontec Dec 1, 2023

paullinator Dec 2, 2023

swansontec left a comment

swansontec Dec 18, 2023

swansontec Dec 18, 2023

swansontec Dec 18, 2023

paullinator commented Dec 19, 2023

Paul/saved action #574

Paul/saved action #574

Conversation

paullinator commented Nov 30, 2023 • edited Loading

CHANGELOG

Dependencies

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swansontec Dec 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swansontec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paullinator commented Dec 19, 2023

paullinator commented Nov 30, 2023 •

edited

Loading

swansontec Dec 4, 2023 •

edited

Loading