Skip to content
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: custom gas in all networks #1631

Merged
merged 9 commits into from
Aug 2, 2024
Merged

Conversation

estebanmino
Copy link
Contributor

@estebanmino estebanmino commented Jul 19, 2024

Fixes BX-1546, BX-1507
Figma link (if any):

What changed (plus any additional context for devs)

enabling custom gas in all networks for all txs types

Screen recordings / screenshots

https://www.loom.com/share/19d39fed3c1040c29ae00a41eddc8a81

What to test

Copy link

linear bot commented Jul 19, 2024

Copy link

Here's the packed extension for this build:
node_modules.tar.gz
rainbowbx-d3e096ecac23ecd2fb5a51cbf81e97a270d3a858

Comment on lines 214 to 216
const [gasPriceWarning, setGasPriceWarning] = useState<
'stuck' | 'fail' | undefined
>(undefined);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can do something like this ?

const [gasPriceWarning, setGasPriceWarning] = useState<'stuck' | 'fail'>();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

Comment on lines 221 to 230
useEffect(() => {
if (
!gasData ||
!enabled ||
prevDebouncedGasPrice === debouncedGasPrice ||
feeType !== 'legacy' ||
!nativeAsset
) {
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use useQuery here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm yeah let me try refactoring this

Copy link
Contributor

@magiziz magiziz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! Just a few questions.

Copy link

linear bot commented Jul 22, 2024

@estebanmino
Copy link
Contributor Author

@magiziz I just applied your comments! 🙏

Copy link

Here's the packed extension for this build:
node_modules.tar.gz
rainbowbx-cffef114e6e008c50b282d443ad1d0790020a478

Copy link

Here's the packed extension for this build:
rainbowbx-cffef114e6e008c50b282d443ad1d0790020a478

@BrodyHughes
Copy link
Member

BrodyHughes commented Aug 1, 2024

Functionality seems good to me. For this sort of case we should do a '<0.001' or something though:

Screenshot 2024-08-01 at 4 06 23 PM

@DanielSinclair
Copy link
Collaborator

@BrodyHughes Which network was that? Wonder if it is a bad result from Backend

@DanielSinclair
Copy link
Collaborator

Lgtm. Have a few design tweaks for existing Custom Gas UI problems, so will create a seperate polish ticket for that and carry over Brody's issue

@DanielSinclair DanielSinclair added this pull request to the merge queue Aug 1, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 1, 2024
@DanielSinclair DanielSinclair added this pull request to the merge queue Aug 2, 2024
Merged via the queue into master with commit 45aad70 Aug 2, 2024
17 checks passed
@DanielSinclair DanielSinclair deleted the @esteban/enable-custom-gas branch August 2, 2024 06:58
Copy link

github-actions bot commented Aug 2, 2024

Here's the packed extension for this build:
node_modules.tar.gz
rainbowbx-45aad704090b76eec0e9c43ee2c1ddb2383cd2a5
screenshots

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants