Skip to content

Conversation

@Cashmaney
Copy link
Member

Added the check_gas API and a couple of tests. Found some interesting behaviour, but otherwise seems pretty solid.

Matching Cosmwasm PR: scrtlabs/cosmwasm#14

cc: @darwinzer0 @blake-regalia - what do you guys think?

@darwinzer0
Copy link
Contributor

darwinzer0 commented Mar 30, 2023

Added the check_gas API and a couple of tests. Found some interesting behaviour, but otherwise seems pretty solid.

Matching Cosmwasm PR: scrtlabs/cosmwasm#14

cc: @darwinzer0 @blake-regalia - what do you guys think?

Looks great! I don't fully understand why it is offset by 1 sometimes. I would have thought since it is always adding a multiple of 1000 to cosmwasm gas that it would eventually result in the same sdk gas when it is divided by 1000 later on, unless there are other things contributing to cosmwasm gas when the api call is made (eg memory reads?).

Anyway, I guess it doesn't matter as long as when a contract dev wants to target exact gas for different functions that they all end up the same (and not some one off from the others).

Do you think it would be useful to create a new SNIP with guidelines on how to use the evaporate API? It could also be incorporated into snip20-reference-impl. Maybe just add an optional exact_gas field to all message types.

@darwinzer0
Copy link
Contributor

darwinzer0 commented Mar 30, 2023

Oh wait, could it just be something as simple as the line Ok(Response::default()) after the API call bumps it over to +1 sdk gas because of some inline gas added for that bit of wasm execution?

@Cashmaney
Copy link
Member Author

Yeah it's possible - let me see if that makes a difference. Though I'd still expect the shift in gas to be consistent if that was the case. But for some reason executing with 100 gas to evaporate returns X and 400000 returns X+1. Maybe it's the difference in some internal allocation in the contract? @assafmo want to take a look at see if you have any other ideas?

Copy link
Contributor

@assafmo assafmo left a comment

Choose a reason for hiding this comment

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

LGTM. The off by 1 issue is definitely something with additional WASM opcodes/memory allocations.

@Cashmaney Cashmaney merged commit cda0935 into master Mar 31, 2023
@Cashmaney Cashmaney deleted the evaporate branch March 31, 2023 09:57
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