-
Notifications
You must be signed in to change notification settings - Fork 4
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
Tx display info refactoring #595
Conversation
@@ -17,6 +17,7 @@ struct WalletTransactionDisplayInfo: Hashable, Identifiable { | |||
let link: URL? | |||
let imageUrl: URL? | |||
let symbol: String | |||
let chain: String |
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.
To be precise, let's call it chainName
. Though having chainID would be the perfect thing to do -- mapping symbol
from the response into a proper value representing a chain
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'll update property to chainName
. chainId
to be discussed
|
||
extension WalletTransactionDisplayInfo { | ||
var chainFullName: String { | ||
if let blockchainType = BlockchainType(rawValue: chain) { |
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.
This line and line 135 repeat the code because the chain
property just copies the value from response and doesn't map it into our app's data management. Let's think about it again
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.
The issue is much deeper.
We used to have one enum that reflected blockchains we're working with: ETH, MATIC, ZIL. It is due to the fact that our domains only existing on this chains.
Now, we have different set of supported chains for number of cases.
- All wallets now also include balances and txs for Base chain
- ULW also support BTC and SOL
- Send crypto feature by imported wallets has own set of supported chains
It is not possible to mix them, and therefore there's places where multiple use cases collides.
In this particular case, tx details, they can be from chains that our BlockchainType does not cover. And this functions abstracting it so no other places in the app aware of this details.
I'm open to suggestions how it can be improved in this particular case without mutating existing core structures like BlockchainType. It seems to me the issue should be resolved on the level deeper.
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.
@Oleg-Pecheneg agreed to make a deeper refactoring. Please create me a ticket, we'll discuss details tomorrow. Thanks!
…/MOB-2060-Send-Crypto-on-Base-chain * commit '9bf5bc72c5ae8c7aeeb6f0d19c0b075e46cf5ec1': (24 commits) Fixed tests target (#599) Update WC SDK to 1.19.4 (#598) MOB-2092 - Fixed copies and domain resolution (#597) MOB-2098 - Show all available ULW tokens in share screen (#596) Tx display info refactoring (#595) Disable LD noisy logs MOB-2092 - Created UI to show TX details (#594) MOB-2086 - Update Fireblocks SDK to v. 2.6.0 (#593) MOB-2090 - Fixing WC requests pull ups UI (#592) MOB-2083 - Support sign typed data via ULW (#591) changed to Float (#590) added 'eth_signTypedData_v4' to optional methods Fixed raw wallet address input formatting (#589) Let users send crypto to themself (#587) MOB-2078 - Fixed regex pattern validation (#588) MOB-2078 - Support sending to none EVM addresses like SOL and BTC for ULW (#586) MOB-2028 - Purchase UW (#575) MOB-2076 - Handle USDC asset in ULW (#583) Always show MATIC USDC in the list of tokens. (#585) MOB-2074 - Update ULW name and appearance in the list (#584) ...
No description provided.