[SUPPORT] remove unnecessary check on tx networkInfo to render SendRowsFee #4010
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
3 tasks
hedi-edelbloute
approved these changes
Jul 13, 2023
hedi-edelbloute
approved these changes
Jul 13, 2023
chabroA
force-pushed
the
support/refacto-LLM-SendRowsFee
branch
from
3280bb2
to
ef15d4a
Compare
not all crypto transaction types have a networkInfo property and some of them have some custom SendRowsFee component If a family SendRowsFee component display is dependent on the presence of a networkInfo property in the transaction object, these families own SendRowsFee component will never be displayed
chabroA
force-pushed
the
support/refacto-LLM-SendRowsFee
branch
from
ef15d4a
to
a6edf06
Compare
|
cherry-picked on develop and moved in this PR #4020 to make it independent from gas tracker work |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
📝 Description
Following this comment #3945 (comment) and the related discussion, proposal to remove what seem to be an unnecessary condition on the existence of a
networkInfoproperty on thetransactionobject to render (or not) a family customSendRowsFeecomponent.Not all crypto transaction types have a
networkInfoproperty and some of them have some customSendRowsFeecomponent.If a family
SendRowsFeecomponent display is dependent on the presence of anetworkInfoproperty in thetransactionobject, these families ownSendRowsFeecomponent will never be displayed.This condition seems odd because not all families have a
networkInfoproperty in theirtransaction. For examplealgorand, but also the newevm. But these families have their ownSendRowsFeecomponent defined in their family folder. Same thing forcelo,polkadot,solana(here we even have some hardcoded patch, cf. this).So, based on this logic, as of today the
SendRowsFeeforalgorand,celoandpolkadotwould not be displayed.Furthermore, we don't seem to have similar logic on LLD (cf. here and here) where we don't have an condition on
networkInfo.The only place where I found such condition on LLD are these 2 files (here and here) related to swap, which might probably be some legacy logic from original swap implementation since these 2 conditions control the display of the
SendAmountFieldsfile listed just above, and it would not make sense to always display this component on a normal send flow but not on a swap flow (if I can edit fees on a normal send, why could I not do it in a Swap flow?)❓ Context
LLMLLD✅ Checklist
📸 Demo
🚀 Expectations to reach
Please make sure you follow these Important Steps.
Pull Requests must pass the CI and be internally validated in order to be merged.