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

PassAll gas option for program to program calls #1417

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

Conversation

iFrostizz
Copy link
Contributor

@iFrostizz iFrostizz commented Aug 20, 2024

Closes #1184

Create a new Gas type that replaces the current alias by an enum that can either be PassAll or Units(NonZeroU64). The new PassAll works around the current way of spending gas, namely calling ConsumeFuel with the passed Fuel parameter and refunding any unused gas by calling AddFuel after the execution.

Copy link
Contributor

@samliok samliok left a comment

Choose a reason for hiding this comment

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

I think this PR's functionality is important but I don't think a Gas enum is the best way to do it. It seems much more intuitive to have gas be a field of context. This way we can keep the u64 type(which is nice for dev experience and contract readability) and also have access to the actual amount of gas via the context.

@iFrostizz
Copy link
Contributor Author

Good point @samliok ! Thanks

@iFrostizz
Copy link
Contributor Author

iFrostizz commented Aug 22, 2024

Actually I'm not sure if that's totally correct. We already have the remaining_fuel method in the implementation of Program but it's not gonna work because it's the full amount of gas is passed and there isn't really any way to predict how much is the external call going to spend (which is the point of this PR) so essentially we want a placeholder that does that for us, which would pass remaining_fuel() - external_call_fuel_cost

}
}

pub fn with_max_units(mut self, units: u64) -> Self {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be great if this function could only accept units that are > 0, there might be some semantic changes to do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Otherwise, replicate the Gas enum type on the go side too so that when it's the Unit variant then it's only interpreted as "pass x units" even when it's 0 to avoid any runtime panic because of the conversion from a u64 to a NonZeroU64.

Copy link
Contributor Author

@iFrostizz iFrostizz Sep 25, 2024

Choose a reason for hiding this comment

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

We might also want to replace the call_contract function by a function that only accepts the Address, function_name, args.

Copy link
Contributor Author

@iFrostizz iFrostizz Sep 25, 2024

Choose a reason for hiding this comment

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

To keep being explicit on the gas forwarding, having a function named with_max_units(u64) and with_pass_all_gas() would make sense. The default behaviour could be to use Gas::PassAll, so the with_pass_all_gas could be ignored.

Comment on lines +48 to 52
// true if the call passes all gas
PassAll bool

// the maximum amount of fuel allowed to be consumed by wasm for this call
Fuel uint64
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would be interested to know if there could be a cleaner solution. Currently there is a bool and the uint64 that tells the amount of fuel passed, but since having PassAll = true and Fuel > 0 is inconsistent state, this has to be validated through the execution. Is there a better way to simulate this rust enum:

enum Gas {
    PassAll,
    Units(u64)
}

@iFrostizz
Copy link
Contributor Author

iFrostizz commented Sep 30, 2024

Passing the units of gas to program highest-level calls to the simulator is still required. We might want to set the units of gas in the simulator initialisation but that sounds a little out of scope for this PR.

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.

Pass all gas parameter in ExternalCallContext
2 participants