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

Add CosmOrc test for NFT mint, transfer, and instantiate gas usage. #84

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

Conversation

0xekez
Copy link
Collaborator

@0xekez 0xekez commented Oct 6, 2022

This adds @de-husk's amazing cosmwasm orc library to this repository which allows us to run contract tests against a real local chan and measure gas usage while we're at it. It currently tests a simple instantiate, mint, and transfer flow and captures gas usage in ci/integration-tests/gas_reports.

Here is an example gas report showing the gas usage of each operation:

{
  "cw721_base": {
    "Execute__cw721_base_mint": {
      "file_name": "ci/integration-tests/src/tests/cw721_base_test.rs",
      "gas_used": 140453,
      "gas_wanted": 186171,
      "line_number": 39
    },
    "Execute__cw721_base_transfer": {
      "file_name": "ci/integration-tests/src/tests/cw721_base_test.rs",
      "gas_used": 137331,
      "gas_wanted": 181488,
      "line_number": 55
    },
    "Instantiate__cw721_base_init": {
      "file_name": "ci/integration-tests/src/tests/cw721_base_test.rs",
      "gas_used": 165333,
      "gas_wanted": 223490,
      "line_number": 21
    }
  }
}

Merging this will cause the associated GitHub action to make a comment on new changes describing how gas usage changed for its tests:

image

@0xekez 0xekez force-pushed the zeke/gas-measurements branch 5 times, most recently from bfdbe1d to 5627cf5 Compare October 6, 2022 19:19
@0xekez 0xekez marked this pull request as ready for review October 6, 2022 19:33
@0xekez 0xekez requested review from JakeHartnell and shanev October 6, 2022 19:34
JakeHartnell
JakeHartnell previously approved these changes Oct 6, 2022
Copy link
Collaborator

@JakeHartnell JakeHartnell left a comment

Choose a reason for hiding this comment

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

Awesome!

@0xekez 0xekez force-pushed the zeke/gas-measurements branch from 2229636 to aaaaf91 Compare October 6, 2022 19:52
JakeHartnell
JakeHartnell previously approved these changes Oct 6, 2022
de-husk
de-husk previously approved these changes Oct 11, 2022
Copy link

@de-husk de-husk left a comment

Choose a reason for hiding this comment

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

Looks great!

Just a minor comment.

@mikedotexe
Copy link
Contributor

Extremely useful, thanks Ekez

JakeHartnell
JakeHartnell previously approved these changes Oct 17, 2022
@ethanfrey
Copy link
Contributor

This orc things looks cool.

Is it ready for larger use? Can you link it here? CosmWasm/cw-plus#507

We had been discussing some way to benchmark gas better, but in the end it needs a chain, not just a local vm, to do this proper.

@JakeHartnell
Copy link
Collaborator

This orc things looks cool.

Is it ready for larger use? Can you link it here? CosmWasm/cw-plus#507

We had been discussing some way to benchmark gas better, but in the end it needs a chain, not just a local vm, to do this proper.

Done. I think it's ready?

Copy link
Contributor

@ueco-jb ueco-jb left a comment

Choose a reason for hiding this comment

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

Really nice example of gas benchmark integration 👍 Gotta save link for later

ci/integration-tests/src/helpers/chain.rs Outdated Show resolved Hide resolved
Comment on lines +3 to +7
e2e integration tests with gas profiling.

`cd ci/integration_tests && cargo t` to run all tests.

`cargo t fn_test_name` to run individual integration tests.
Copy link
Contributor

Choose a reason for hiding this comment

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

Also some CONFIG variable needs to be set.
https://github.com/CosmWasm/cw-nfts/pull/84/files#diff-1d721ea28fc40ef113f3080306e4595d8176b794321429300530a386a0e305c6R83

and I don't see any mention about that (I might be blind though).

@JakeHartnell JakeHartnell dismissed stale reviews from de-husk and themself via 48a8823 October 24, 2022 20:56
@0xekez 0xekez force-pushed the zeke/gas-measurements branch from 2a26f4f to 7e96526 Compare October 24, 2022 21:07
Copy link
Member

@shanev shanev left a comment

Choose a reason for hiding this comment

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

Should totally get this in after it's been updated.

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.

7 participants