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

[JSS-101] Estimate smart-contract update energy costs #362

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Ivan-Mahda
Copy link
Contributor

@Ivan-Mahda Ivan-Mahda commented May 22, 2024

Purpose

We should provide functions for computing an estimate of energy usage needed for running an update transaction for a smart contract on chain.
The function should take the information needed for the update and use invokeContract to compute the energy cost.

Changes

Added new method getContractUpdateEnergyCost
Get contract update energy cost
Estimated by calculateEnergyCost, where transactionSpecificCost received from invokeContract used energy

Actual energy can be different from the estimated (about 1 - 5 NRG)

Updated README.md for node-sdk examples

image

Checklist

  • My code follows the style of this project.
  • The code compiles without warnings.
  • I have performed a self-review of the changes.
  • I have documented my code, in particular the intent of the
    hard-to-understand areas.
  • (If necessary) I have updated the CHANGELOG.

By submitting the contribution I accept the terms and conditions of the
Contributor License Agreement v1.0

Added new method getContractUpdateEnergyCost
Get contract update energy cost
Estimated by calculateEnergyCost, where transactionSpecificCost received from invokeContract used energy

Updated README.md for node-sdk examples
@Ivan-Mahda Ivan-Mahda requested review from soerenbf and limemloh May 22, 2024 09:36

Default endpoint for node is 'localhost:20000',
but instead of installing local node,
can be used testnet node <https://node.testnet.concordium.com:20000>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This endpoint has been replaced

Suggested change
can be used testnet node <https://node.testnet.concordium.com:20000>
can be used testnet node <https://grpc.testnet.concordium.com:20000>

@@ -2,6 +2,9 @@
"ignorePatterns": [
{
"pattern": "classes/grpc.ConcordiumGRPCClient.html"
},
{
"pattern": "https://node.testnet.concordium.com:20000"
Copy link
Collaborator

Choose a reason for hiding this comment

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

We are migrating to another endpoint

Suggested change
"pattern": "https://node.testnet.concordium.com:20000"
"pattern": "https://grpc.testnet.concordium.com:20000"

* @param {Parameter.Type} parameter - Input for contract function
* @param {ReceiveName.Type} method - Represents a receive-function in a smart contract module
* @param {bigint} signatureCount - Number of expected signatures
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggestion: Please document when this function throws and also that the estimate is given for the last finalized block according to the node, which means this the actual energy cost might be different depending on the implementation of the smart contract and the interaction with the instance, since this was estimated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it not make sense to add an optional blockHash argument which can be passed to the grpc client?

changed test node link

updated getContractUpdateEnergyCost description
Copy link
Collaborator

@soerenbf soerenbf left a comment

Choose a reason for hiding this comment

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

Mainly comments regarding how we usually bump/release

export function getUpdatePayloadSize(
parameterSize: number,
receiveNameLength: number
) {
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
) {
): bigint {

@@ -1,6 +1,6 @@
{
"name": "@concordium/web-sdk",
"version": "7.4.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would argue that this is a minor version (as it adds new functionality). The way we usually release though, is that we create a pull request specifically for bumping the version in preparation for the release. This is to avoid confusion with regards to what has already been released and what is pending release.

@@ -1,5 +1,11 @@
# Changelog

## 7.4.1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consequently, this should be

Suggested change
## 7.4.1
## Unreleased

* @param {Parameter.Type} parameter - Input for contract function
* @param {ReceiveName.Type} method - Represents a receive-function in a smart contract module
* @param {bigint} signatureCount - Number of expected signatures
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it not make sense to add an optional blockHash argument which can be passed to the grpc client?

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