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

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

Merged

Conversation

akhi3030
Copy link
Collaborator

@akhi3030 akhi3030 commented May 5, 2022

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
Copy link
Collaborator Author

akhi3030 commented May 5, 2022

@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
Copy link
Contributor

jakmeier commented May 5, 2022

@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
Copy link
Collaborator Author

akhi3030 commented May 5, 2022

@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
Copy link
Contributor

jakmeier commented May 5, 2022

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
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