Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Feat/issue 1026/gas complete implementation #1122

Merged
merged 34 commits into from
Oct 17, 2024

Conversation

lorenzogentile404
Copy link
Collaborator

No description provided.

@lorenzogentile404 lorenzogentile404 linked an issue Sep 6, 2024 that may be closed by this pull request
@OlivierBBB OlivierBBB enabled auto-merge (squash) September 6, 2024 11:51
@OlivierBBB
Copy link
Collaborator

I guess what is missing is the triggering. I expect that we can have the triggering happen in the CommonValuesFragment (IIRC this is where we compute the CONTEXT_MAY_CHANGE flag which is the trigger for this module.)

Copy link
Collaborator

@letypequividelespoubelles letypequividelespoubelles left a comment

Choose a reason for hiding this comment

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

LGTM, two philosophical comments

public void call(GasCall gasCall, Hub hub, CommonFragmentValues commonValues) {
this.commonValues = commonValues;
this.gasCall = gasCall;
hub.defers().scheduleForPostExecution(this);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I find VERy weird to schedule for post exec teh GAS module and not the GasOperation ... Of course it works but .... strange.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How would you do it to make it look more natural?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see any beautiful and nice way to do it wo copying duplicate info and add cross-link ... Let's keep it like this :)

wcpRes[1] = true;
final boolean gasCostIsNonNegative = wcp.callLEQ(0, gasCall.getGasCost().longValue());
wcpRes[1] = gasCostIsNonNegative; // true
checkArgument(gasCostIsNonNegative);
Copy link
Collaborator

Choose a reason for hiding this comment

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

are the checkARgument really usefull ? It'll break on the constraint check anyway. It's making useless check imho

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right.

@OlivierBBB
Copy link
Collaborator

We have a failing constraint

gas.asserting-either-sufficient-gas-or-insufficient-gas

@lorenzogentile404
Copy link
Collaborator Author

Screenshot 2024-10-16 at 14 55 50
we also get this Java Exception.

@OlivierBBB OlivierBBB merged commit e1ef888 into arith-dev Oct 17, 2024
5 checks passed
@OlivierBBB OlivierBBB deleted the feat/issue-1026/gas-complete-implementation branch October 17, 2024 14:08
OlivierBBB added a commit that referenced this pull request Oct 17, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(gas): complete implementation
3 participants