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: added a new flow to let users manually configure transactions' gas limit on the System Contract DApp #678

Merged
merged 15 commits into from
Feb 20, 2024

Conversation

quiet-node
Copy link
Member

Description:
This PR has implemented a new gas estimation mechanism for the System Contract DApp Playground. With this enhancement, users can now specify custom gas limits for transactions manually. In cases where no gas limits are provided, the transactions' calldata will be forwarded to a mirror-node for calculating the necessary gas limit. This improved flow is now accessible to all transactions undergoing consensus.

Related issue(s):

Fixes #668

UI Updates:

Transactions will now have an extra textbox next to their execute buttons which receive custom gas limits.

image

image

image

image

Notes for reviewer:

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

@quiet-node quiet-node self-assigned this Feb 16, 2024
@quiet-node quiet-node requested a review from a team as a code owner February 16, 2024 14:29
@quiet-node quiet-node requested a review from acuarica February 16, 2024 14:29
@quiet-node quiet-node linked an issue Feb 16, 2024 that may be closed by this pull request
@quiet-node quiet-node marked this pull request as draft February 16, 2024 14:36
Copy link

github-actions bot commented Feb 16, 2024

Test Results

242 tests  ±0   236 ✔️ ±0   7m 53s ⏱️ +28s
  72 suites ±0       6 💤 ±0 
  14 files   ±0       0 ±0 

Results for commit 70a2f5e. ± Comparison against base commit 56ecc17.

♻️ This comment has been updated with latest results.

Signed-off-by: Logan Nguyen <[email protected]>
@quiet-node quiet-node marked this pull request as ready for review February 16, 2024 15:17
@quiet-node quiet-node changed the title feat: added a new flow to let users manually configure transactions' gas limit feat: added a new flow to let users manually configure transactions' gas limit on the System Contract DApp Feb 16, 2024
Copy link
Collaborator

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

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

LG
Minor edit but repeated many times

MOCK_TX_HASH,
MOCK_GAS_LIMIT,
MOCK_HEDERA_NETWORK,
MOCK_SINGER_ADDRESS,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
MOCK_SINGER_ADDRESS,
MOCK_SIGNER_ADDRESS,

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh thanks for the nice catch!

acuarica
acuarica previously approved these changes Feb 16, 2024
Copy link
Contributor

@acuarica acuarica left a comment

Choose a reason for hiding this comment

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

lgtm, left some details comments

'cryptoTransferPublic',
[transferList, tokenTransferList]
);
if (!estimateGasResult.gasLimit || estimateGasResult.err) return { err: estimateGasResult.err };
Copy link
Contributor

Choose a reason for hiding this comment

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

I see this logic is duplicated in each of these APIs, do you think is worth to refactor it to avoid this duplication?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a great suggestion Luis, I appreciate it! You're right this logic recurs in most of the API calls. I've devised another plan to address this by crafting a more robust error handler, which I'll integrate into another PR. 😄

accountId?: string;
contractId?: string;
err?: any;
}

/**
* @dev an interface for the results returned back from querying estimated gasLimit to the Mirror Node
Copy link
Contributor

Choose a reason for hiding this comment

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

small detail: Is @dev a valid TSDoc https://tsdoc.org/ annotation?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh you're right at the beginning I just brought this annotation over from Solidity and didn't do any research on this! Will create a ticket for this soon! Thanks a lot for pointing it out!

*/
interface IEstimateGasMirrorNodeResult {
gasLimit?: number;
err?: any;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to write a small explanation on why err is any here? In other words, where err does come from? Something like, the type of Exception that has been thrown when doing the request to the mirror node.

Copy link
Member Author

Choose a reason for hiding this comment

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

Excellent suggestion! This I will keep in mind when I integrate the new error handler I mentioned above! Thanks Luis!

Copy link
Contributor

@acuarica acuarica left a comment

Choose a reason for hiding this comment

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

lg

@quiet-node quiet-node merged commit 1d07283 into main Feb 20, 2024
23 checks passed
@quiet-node quiet-node deleted the 668-system-contract-dapp-remove-gas-input-option branch February 20, 2024 17:17
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.

[System Contract DApp] Mark manual gasLimit input optional
3 participants