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

dry run transactions prior to execution #10365

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Moliholy
Copy link
Contributor

@Moliholy Moliholy commented Mar 18, 2024

This PR adds dry run check prior to extrinsic submission and prints an alert if the extrinsic would fail in its execution. Useful to warn the user before needlessly spending network fees.

Screenshots:

Screenshot 2024-03-18 at 08 47 48

@Moliholy
Copy link
Contributor Author

@TarikGul any feedback for the PR? do you think it's a good idea?

@@ -67,6 +79,9 @@ function PaymentInfo ({ accountId, className = '', extrinsic, isHeader }: Props)
{isFeeError && (
<MarkWarning content={t('The account does not have enough free funds (excluding locked/bonded/reserved) available to cover the transaction fees without dropping the balance below the account existential amount.')} />
)}
{isDryRunError && (
<MarkError content={t('The transaction would not be successfully executed')} />
Copy link
Member

Choose a reason for hiding this comment

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

I would ensure we also bring up that it failed via the dry run.

So something like: The transaction did not successfully pass a dry run, something something

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just changed it to The transaction did not pass a dry run and would likely not be successfully executed. Is that OK?

@TarikGul
Copy link
Member

Apologies on the delay, I haven't gotten around to truly running this yet. I do think it is a good idea though, i just need to test it locally as well!

@Moliholy
Copy link
Contributor Author

No worries! I just updated the text and rebased to the latest master branch.

@TarikGul
Copy link
Member

I haven't forgotten about this PR :), I'll have a response soon but there was some internal discussion on our team on some potential problems that this can introduce.

It's definitely a wanted feature so I would love to see this get in, but we just need to ensure the edge cases are resolved (to be written out soon)

@Moliholy
Copy link
Contributor Author

Thanks for the update. This is actually a small part of a bigger task that I had in mind. In a real-world production scenario a user would not have the private key accessible in the browser and instead would likely be using wallets like Talisman. In such case the account will very likely be locked and hence the feature implemented here would be useless, as it would not prevent real funds from being spent.

For that case I thought to implement a Dry run button near the Sign and submit one that would require the user to unlock the account and execute the dry run operation on demand. But before that I wanted to see how was the reception of this PR, as I supposed it could bring some friction.

@IkerAlus
Copy link

Important to note that this feature implies calling an unsafe RPC method and most likely won't be available in any public provider.

@Moliholy
Copy link
Contributor Author

Moliholy commented Apr 18, 2024

Which is, since we're on it, a huge disadvantage IMO comparing to Ethereum. See here:

Executes a message call transaction, which is directly executed in the VM of the node, but never mined into the blockchain.

If you can query the chain state like in Ethereum while spending computational resources, why can't you dry-run a transaction also spending computational resources?

Actually, I hoped implementing this feature (and others that might be coming) could make some pressure to the community so that by default there exists a way for any client to perform dry-runs of their transactions.

Anyhow... the hasDryRun boolean value is supposed to indicate whether this method is allowed or not. Also please take into account that in most (all?) cases it will work if using zombienet.

@IkerAlus
Copy link

@Moliholy Good points, it may be worth it to revive this old issue in Polkadot-SDK.

@xlc
Copy link
Contributor

xlc commented May 28, 2024

Note that the dry run RPC isn't that useful. See this for more details paritytech/polkadot-sdk#341
Once we have the new dry run runtime API, we should use it instead paritytech/polkadot-sdk#4432
Otherwise should use Chopsticks to dry run locally. However it can take up to a few minutes so can't be enabled by default.

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

Successfully merging this pull request may close these issues.

4 participants