Skip to content

Conversation

gligneul
Copy link
Contributor

@gligneul gligneul commented Jul 4, 2025

No description provided.

@cla-bot cla-bot bot added the s CLA signed label Jul 4, 2025
@MishkaRogachev MishkaRogachev mentioned this pull request Jul 4, 2025
7 tasks
@eljobe
Copy link
Member

eljobe commented Jul 7, 2025

@gligneul, are you ready for review? If so, can you click, "Ready for review", give it a proper PR description and assign this to me?
If not, what's still outstanding?

@gligneul
Copy link
Contributor Author

gligneul commented Jul 7, 2025

@gligneul, are you ready for review? If so, can you click, "Ready for review", give it a proper PR description and assign this to me?
If not, what's still outstanding?

There are quite a few things missing. I will open it soon.

gligneul added 2 commits July 7, 2025 11:38
Add a function that only increments a single resource kind instead of
adding up two multi-gas.
Also checks for overflows in the functions.
@gligneul gligneul marked this pull request as ready for review July 7, 2025 14:40
@gligneul gligneul requested review from MishkaRogachev and eljobe July 7, 2025 14:40
@gligneul
Copy link
Contributor Author

gligneul commented Jul 7, 2025

I decided to open this PR with just the multigas methods. Later, I will open another PR passing the multigas variable to the interpreter.

MishkaRogachev
MishkaRogachev previously approved these changes Jul 7, 2025
Copy link
Contributor

@MishkaRogachev MishkaRogachev left a comment

Choose a reason for hiding this comment

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

Overall everything looks good, there is one small suggestion:

func (mg *MultiGas) ToLegacyGas() uint64 {
   var sum uint64
   for _, value := range mg.gas {
   	sum = math.SafeAdd(sum, value)
   }
   return sum
}

it will help me in the tests

@eljobe
Copy link
Member

eljobe commented Jul 7, 2025

Overall everything looks good, there is one small suggestion:

func (mg *MultiGas) ToLegacyGas() uint64 {
   var sum uint64
   for _, value := range mg.gas {
   	sum = math.SafeAdd(sum, value)
   }
   return sum
}

it will help me in the tests

I would like to not call such a method ToLegacyGas because it makes it seem like we are no longer using that kind of gas. But, in fact, single-dimensional gas is still a valid concept in the system.
Let's go with SingleDimGas() or SingleGas() or Aggregate() or something like that.

eljobe
eljobe previously approved these changes Jul 7, 2025
@eljobe eljobe self-assigned this Jul 7, 2025
@gligneul gligneul dismissed stale reviews from eljobe and MishkaRogachev via 23ea7d7 July 7, 2025 17:46
@eljobe eljobe enabled auto-merge July 7, 2025 18:09
@eljobe eljobe merged commit db018f8 into master Jul 7, 2025
14 checks passed
@eljobe eljobe deleted the gligneul/multigas branch July 7, 2025 18:19
eljobe added a commit that referenced this pull request Jul 18, 2025
@eljobe eljobe mentioned this pull request Jul 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants