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

R4R: metatx add from field into MetaTxSignData, so that to sign hash with from's addr #37

Merged
merged 12 commits into from
Feb 6, 2024

Conversation

wwqicode-blkchain
Copy link

No description provided.

@Tri-stone
Copy link

It's a hard fork modification. Before and after the modification, meta transaction are decoded into different data struct.

  • before the modification, use the former MetaTxSignData without From to encode and decode meta transaction

@abelliumnt
Copy link

Please fix failed unit tests in meta_transaction_test.go

func TransactionToMessage(tx *types.Transaction, s types.Signer, baseFee *big.Int) (*Message, error) {
metaTxParams, err := types.DecodeAndVerifyMetaTxParams(tx)
func TransactionToMessage(tx *types.Transaction, s types.Signer, baseFee *big.Int, isMetaTxUpgraded bool) (*Message, error) {
metaTxParams, err := types.DecodeAndVerifyMetaTxParams(tx, isMetaTxUpgraded)

Choose a reason for hiding this comment

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

Adding parameter isMetaTxUpgraded bool to TransactionToMessage affect so many codes. However, isMetaTxUpgraded is just for fixing a bug in metaTx. If we need new upgrade config in TransactionToMessage, more parameters are required.
So I suggest to add a more general parameter here, like chainConfig. Thus, in the future, if we need more upgrade here, it is not necessary to add more parameters here.

Copy link
Author

Choose a reason for hiding this comment

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

Yes we can add a configuration for easier access to the base values in case of future code changes. However, we still need to pass the corresponding real values and compare them with the configuration values. What if we change the parameter to TransactionToMessage(xxx, config params.ChainConfig, isMetaTxUpgraded bool, inters ...interface{}) ?

@wwqicode-blkchain wwqicode-blkchain force-pushed the wwq/metatx-signFromAddr branch 2 times, most recently from eb393ed to f6e5382 Compare February 5, 2024 06:15
params/config.go Outdated
@@ -138,6 +138,7 @@ var (
MergeNetsplitBlock: big.NewInt(1735371),
ShanghaiTime: newUint64(1677557088),
Ethash: new(EthashConfig),
MetaTxUpgradeTime: newUint64(0), // newUint64(1707152400),

Choose a reason for hiding this comment

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

please delete this config, it's useless.

Copy link
Author

Choose a reason for hiding this comment

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

done

params/config.go Outdated
@@ -682,6 +686,11 @@ func (c *ChainConfig) IsMantleBVMETHMintUpgrade(time uint64) bool {
return isTimestampForked(c.BVMETHMintUpgradeTime, time)
}

// IsMetaTxV2 returns whether time is either equal to the MetaTx fork time or greater.
func (c *ChainConfig) IsMetaTxV2(time uint64) bool {
return isMetaTxForked(c.MetaTxUpgradeTime, time)
Copy link

@Tri-stone Tri-stone Feb 5, 2024

Choose a reason for hiding this comment

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

please use isTimestampForked instead of isMetaTxForked and delete isMetaTxForked

Tri-stone
Tri-stone previously approved these changes Feb 5, 2024
@wwqicode-blkchain wwqicode-blkchain merged commit 4d05b2c into release/v0.5.0 Feb 6, 2024
2 checks passed
Tri-stone added a commit that referenced this pull request Mar 1, 2024
[R4R]-feat: merge from release/v0.5.0 to develop
Core changes:
- [R4R]feat: mint eth to msg.from and transfer revertable #31
- [R4R]feat: metatx add from field into MetaTxSignData, so that to sign hash with from's addr #37
- [R4R]feat: add error message when failing to forward tx to sequencer #39
- [R4R]fix: fix tx cost #41
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