refactor: remove usage of __typename for the tx feed
#6186
Conversation
jeanregisser
force-pushed
the
jeanregisser/remove-typename
branch
from
96934e5 to
c90411f
Compare
| origin: 'EarnDeposit' | 'EarnWithdraw' | 'EarnClaimReward' | 'EarnSwapDeposit' | ||
| origin: | ||
| | TokenTransactionTypeV2.EarnDeposit | ||
| | TokenTransactionTypeV2.EarnWithdraw | ||
| | TokenTransactionTypeV2.EarnClaimReward | ||
| | TokenTransactionTypeV2.EarnSwapDeposit |
There was a problem hiding this comment.
@satish-ravi do you think this could be a problem?
The value will be different now.
There was a problem hiding this comment.
I guess we generally try to avoid breaking changes for analytics in case there's some dashboard dependencies. We have had instances in the past where we had to workaround to avoid this. But maybe fine here, I don't think this is a critical analytics event. Would be good to confirm with product
There was a problem hiding this comment.
Thanks yes I'll check.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6186 +/- ##
==========================================
- Coverage 88.90% 88.88% -0.02%
==========================================
Files 733 733
Lines 31142 31166 +24
Branches 5717 5736 +19
==========================================
+ Hits 27686 27702 +16
- Misses 3259 3265 +6
- Partials 197 199 +2
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| return <SwapFeedItem key={tx.transactionHash} transaction={tx as TokenExchange} /> | ||
| return <SwapFeedItem key={tx.transactionHash} transaction={tx} /> | ||
| case TokenTransactionTypeV2.Sent: | ||
| case TokenTransactionTypeV2.Received: | ||
| return <TransferFeedItem key={tx.transactionHash} transfer={tx as TokenTransfer} /> | ||
| return <TransferFeedItem key={tx.transactionHash} transfer={tx} /> | ||
| case TokenTransactionTypeV2.NftSent: | ||
| case TokenTransactionTypeV2.NftReceived: | ||
| return <NftFeedItem key={tx.transactionHash} transaction={tx as NftTransfer} /> | ||
| return <NftFeedItem key={tx.transactionHash} transaction={tx} /> | ||
| case TokenTransactionTypeV2.Approval: | ||
| return <TokenApprovalFeedItem key={tx.transactionHash} transaction={tx as TokenApproval} /> | ||
| return <TokenApprovalFeedItem key={tx.transactionHash} transaction={tx} /> | ||
| case TokenTransactionTypeV2.EarnDeposit: | ||
| case TokenTransactionTypeV2.EarnSwapDeposit: | ||
| case TokenTransactionTypeV2.EarnWithdraw: | ||
| case TokenTransactionTypeV2.EarnClaimReward: | ||
| return <EarnFeedItem key={tx.transactionHash} transaction={tx as TokenEarn} /> | ||
| return <EarnFeedItem key={tx.transactionHash} transaction={tx} /> |
There was a problem hiding this comment.
All these casts could be removed now
There was a problem hiding this comment.
Same here, we don't need to cast anymore.
| case 'TokenExchangeV3': | ||
| case 'CrossChainTokenExchange': | ||
| switch (tx.type) { | ||
| case TokenTransactionTypeV2.Exchange: |
There was a problem hiding this comment.
do we need to add this exchange type? was thinking that we could remove it, since it hasn't been queried for in a long time and is not part of the graphql transactions query?
There was a problem hiding this comment.
Not anymore but without it, TS complains, I'm planning to remove this separately.
| @@ -1924,4 +1924,5 @@ export const migrations = { | |||
| feedFirstPage: [], | |||
| }, | |||
| }), | |||
| 235: (state: any) => state, | |||
There was a problem hiding this comment.
do we need a migration for this change? it feels odd to update the root schema without incrementing the schema number but this migration doesn't remove the __typename property on persisted transactions which is the only thing that changes about the state...the persisted transactions are replaced on app start very quickly anyway?
There was a problem hiding this comment.
Yes I decided to increase the schema number (and hence the no-op migration) to make this change more explicit.
But we could have skipped it.
Description
__typenamewas inherited from GraphQL and is not needed anymore.It was duplicating the
typeproperty and making it a bit difficult to see which one should be used now.Test plan
Related issues
N/A
Backwards compatibility
Almost, but the analytics event
earn_feed_item_selectwill now return thetypevalue instead of__typenamefor theoriginproperty.Network scalability
If a new NetworkId and/or Network are added in the future, the changes in this PR will: