Skip to content

Conversation

@lukasz-golebiewski
Copy link
Contributor

No description provided.

@lukasz-golebiewski lukasz-golebiewski force-pushed the feature/etcm-921/more-codes branch from 9bcbb6e to aa23ecf Compare July 30, 2021 13:21
@drospa drospa changed the title [ETCM-921] Magneto gas changes for BALANCE and *CALL codes [ETCM-912] Magneto gas changes for BALANCE and *CALL codes Aug 2, 2021
outOffset: UInt256 = inputData.size,
outSize: UInt256 = inputData.size / 2
outSize: UInt256 = inputData.size / 2,
toAccessed: Boolean = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I had to look into the code to understand what this field was about.
I would have prefered something more descriptive like addAddressToAccessList or keepTrackOfAccessAddresses, but I tend to belong to the long-name-team.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WDYT about toAlreadyAccessed?

Copy link
Contributor

Choose a reason for hiding this comment

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

it seems clearer for me.


import Fixtures.blockchainConfig

class MagnetoCallOpFixture(config: EvmConfig)
Copy link
Contributor

Choose a reason for hiding this comment

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

Most of the tests seem to have be taken directly from CallOpcodesSpec.
It could be nice to keep unmodified tests in one place only, and have specific gas ones separated.

It seems to be possible to have something like that:

  • CallOpCodesCommonBehaviour: with the common tests
  • CallOpCodesSpec: with the pre eip2929 gas tests
  • CallOpCodesSpecPostEip2929: with post eip 2929 gas tests

This kind of separation seems to be possible: https://www.scalatest.org/user_guide/sharing_tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reduced as much duplication as possible, moved common assertions to a trait

postWarmGasFn: FeeSchedule => BigInt
): BigInt = {
val currentBlockNumber = state.env.blockHeader.number
val etcFork = state.config.blockchainConfig.etcForkForBlockNumber(currentBlockNumber)
Copy link
Contributor

@dzajkowski dzajkowski Aug 5, 2021

Choose a reason for hiding this comment

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

do you mind adding a note that ETH needs to be also handled?

Copy link
Contributor

@dzajkowski dzajkowski left a comment

Choose a reason for hiding this comment

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

LGTM

@lukasz-golebiewski lukasz-golebiewski force-pushed the feature/etcm-921/more-codes branch from aa23ecf to 3740dbd Compare August 9, 2021 07:18
@lukasz-golebiewski lukasz-golebiewski force-pushed the feature/etcm-921/more-codes branch from 000dc2d to e487da7 Compare August 9, 2021 13:22
@lukasz-golebiewski lukasz-golebiewski merged commit 241d1f3 into develop Aug 9, 2021
@lukasz-golebiewski lukasz-golebiewski deleted the feature/etcm-921/more-codes branch August 9, 2021 14:51
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.

4 participants