Skip to content

Use correct cost in gas profile for adding function call key - #6749

Merged
near-bulldozer[bot] merged 6 commits into
near:masterfrom
akhi3030:akhi3030/correct-cost-gas-profile
May 9, 2022
Merged

Use correct cost in gas profile for adding function call key#6749
near-bulldozer[bot] merged 6 commits into
near:masterfrom
akhi3030:akhi3030/correct-cost-gas-profile

Conversation

@akhi3030

@akhi3030 akhi3030 commented May 5, 2022

Copy link
Copy Markdown
Contributor

Closes #5961 .

@akhi3030
akhi3030 requested a review from a team as a code owner May 5, 2022 06:56
@akhi3030
akhi3030 requested a review from mzhangmzz May 5, 2022 06:56
@akhi3030

akhi3030 commented May 5, 2022

Copy link
Copy Markdown
Contributor Author

@jakmeier , @Ekleog : I am unable to edit the list of reviewers and assignees. Probably some missing permissions. So pinging you just in case you didn't get a ping already.

@jakmeier

jakmeier commented May 5, 2022

Copy link
Copy Markdown
Contributor

@akhi3030 regarding permissions, I think someone (with more power than me) just needs to add you to the org.

About the code, this is correct in principle but we also have to update tests that check for the exact gas profile. I believe the relevant buildkite pipelines are public, so you should be able to see the failing tests there. Just follow the link to details. (You might need to create an account.)

You can also run tests locally. To get the same features as used in CI (nightly protocol features etc), ideally you just copy each of the failing commands from the CI output.

@akhi3030

akhi3030 commented May 5, 2022

Copy link
Copy Markdown
Contributor Author

@akhi3030 regarding permissions, I think someone (with more power than me) just needs to add you to the org.

Thanks. I've pinged some people to add me to the org.

About the code, this is correct in principle but we also have to update tests that check for the exact gas profile. I believe the relevant buildkite pipelines are public, so you should be able to see the failing tests there. Just follow the link to details. (You might need to create an account.)

You can also run tests locally. To get the same features as used in CI (nightly protocol features etc), ideally you just copy each of the failing commands from the CI output.

Thanks for the info. I'll explore more.

@jakmeier

jakmeier commented May 5, 2022

Copy link
Copy Markdown
Contributor

LGTM

One more thing, probably we should also include this as a non protocol change in CHANGELOG.md. It is observable by users when they query an RPC node for gas usage.

Pinging @marcelo-gonzalez as the 1.26 release owner, can we just add changes to the unreleased section? (Just making sure it doesn't get mixed with changes that made the 1.26 cut)

@marcelo-gonzalez

Copy link
Copy Markdown
Contributor

LGTM

One more thing, probably we should also include this as a non protocol change in CHANGELOG.md. It is observable by users when they query an RPC node for gas usage.

Pinging @marcelo-gonzalez as the 1.26 release owner, can we just add changes to the unreleased section? (Just making sure it doesn't get mixed with changes that made the 1.26 cut)

ahh yes go ahead and add that, totally forgot to udpate the CHANGELOG. Just sent #6757, so should be easier to update the CHANGELOG after that. In case this PR is merged before that one, you can feel free to just add to the unreleased section and i'll handle the merge commit int hat PR

@mzhangmzz
mzhangmzz removed their request for review May 6, 2022 15:28
@near-bulldozer
near-bulldozer Bot merged commit 7cc1971 into near:master May 9, 2022
akhi3030 added a commit that referenced this pull request May 9, 2022
PR #6749 incorrectly updated the CHANGELOG.  This PR moves the change to the right section.
near-bulldozer Bot pushed a commit that referenced this pull request May 9, 2022
…6771)

PR #6749 incorrectly updated the CHANGELOG.  This PR moves the change to the right section.
@akhi3030
akhi3030 deleted the akhi3030/correct-cost-gas-profile branch September 8, 2022 10:08
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.

Use correct cost in gas profile for adding function call key

3 participants