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: show warning if gas exceeds 500_000, refactor state #121

Conversation

gsteenkamp89
Copy link

@gsteenkamp89 gsteenkamp89 commented Jan 31, 2024

closes UMA-2168
closes UMA-2336

motivation

oSnap will only subsidize 500_000 gas and execute transaction automatically. We want to show users a warning if their transaction is exceeding this limit.

Note: This screenshot quotes the limit to be 50_000, but this is only to force showing this warning. The actual figured hardcoded is 500_000.

screenshots

Screenshot 2024-01-31 at 12 58 03

If the tenderly project is not public, don't share link. (this will likely not happen since we control the project and are always interacting with the OG contract)

Screenshot 2024-01-31 at 17 14 20

Copy link

linear bot commented Jan 31, 2024

Copy link

vercel bot commented Jan 31, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
snapshot ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 31, 2024 3:16pm
snapshot-goerli ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 31, 2024 3:16pm

@gsteenkamp89 gsteenkamp89 requested a review from daywiss January 31, 2024 11:01
@daywiss daywiss requested a review from Reinis-FRP January 31, 2024 14:13
@daywiss
Copy link

daywiss commented Jan 31, 2024

@Reinis-FRP can you verify that this gas amount returned from sim is the same gas the bot uses to threshold automatic execution

Comment on lines 183 to 198
<div
class="flex flex-col w-full items-start text-left"
v-if="simulationState.exceedsGasSubsidy"
>
<p class="text-sm">
<strong class="text-skins text-base text-red">Warning:</strong>
This transaction will
<strong class="underline"
>not be automatically executed by oSnap.</strong
>
This transaction used
{{ simulationState.gasUsed.toLocaleString() }} gas, which exceeds
oSnap's maximum subsidized amount of
{{ OSNAP_GAS_SUBSIDY.toLocaleString() }}.
</p>
</div>
Copy link
Author

Choose a reason for hiding this comment

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

just removed the unnecessary div that wraps this text

</div>

<TuneButton
class="group text-sm md:text-[18px] hover:cursor-pointer justify-center w-full flex gap-2 mx-auto items-center"
:tooltip="'Reset Simulation'"
@click="resetState"
>
Reset
Reset Simulation
Copy link
Author

Choose a reason for hiding this comment

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

more explicit copy

@@ -163,39 +163,37 @@ async function simulate() {
>
</div>
<a
v-if="simulationState.simulationLink.public"
Copy link
Author

Choose a reason for hiding this comment

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

if link isn't public, don't share it

Copy link

linear bot commented Jan 31, 2024

Copy link

@daywiss daywiss left a comment

Choose a reason for hiding this comment

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

nice

@Reinis-FRP
Copy link

@Reinis-FRP can you verify that this gas amount returned from sim is the same gas the bot uses to threshold automatic execution

yes, 500K is default value used by execution bot

@@ -53,3 +56,7 @@ export function prepareTenderlySimulationPayload(props: {

return { safe: payload };
}

export function exceedsOsnapGasSubsidy(res: TenderlySimulationResult): boolean {
return res.gasUsed > OSNAP_GAS_SUBSIDY;

Choose a reason for hiding this comment

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

Note that simulation gas usage might be lower than real one due to how state overrides short-circuit the OOv3 settlement logic. The execution bot uses ethers estimateGas on oSnap executeProposal call at the time when challenge window passed, so it might be worth checking this divergence in some test runs

Copy link
Author

Choose a reason for hiding this comment

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

interesting we might want to implement some sort or buffer percentage.

@gsteenkamp89 gsteenkamp89 merged commit d5fc559 into gerhard/uma-2153-ui-render-the-tenderly-simulation-button-in-the-tx-builder Feb 1, 2024
4 checks passed
@gsteenkamp89 gsteenkamp89 deleted the gerhard/uma-2168-add-snapshot-ui-warning-to-gas-limit-when-user-creating branch February 1, 2024 08:51
daywiss added a commit that referenced this pull request Feb 22, 2024
* create base component logic

* update styling

* validate payload, handle request and update styles

* better mobile styles

* use separate reset button

* feat: show warning if gas exceeds 500_000, refactor state (#121)

* feat: add simulation in proposal view (#122)

* feat: fetch implementation abi in transaction builder for proxy contracts (#124)

* use isValid flag to check transactions in simulation

* better feedback for EOA in "to" field & ABI with no write functions

* clean up

* reset parameters if ABI changes

* revert extra green & red variables

* fix typo

* use better icon convention

---------

Co-authored-by: Gerhard Steenkamp <[email protected]>
Co-authored-by: Gerhard Steenkamp <[email protected]>
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.

3 participants