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

fix: data field in CodableTransaction must be passed into the envelope #624

Merged

Conversation

JeneaVranceanu
Copy link
Collaborator

@JeneaVranceanu JeneaVranceanu commented Sep 27, 2022

Potential fix for #619
Still testing.

@JeneaVranceanu JeneaVranceanu changed the title fix: data field in CodableTransaction must be passed into the envelope [WIP] fix: data field in CodableTransaction must be passed into the envelope Sep 27, 2022
@@ -420,8 +416,6 @@ extension CodableTransaction {
chainID: BigUInt = 0, value: BigUInt = 0, data: Data = Data(),
gasLimit: BigUInt = 0, maxFeePerGas: BigUInt? = nil, maxPriorityFeePerGas: BigUInt? = nil, gasPrice: BigUInt? = nil,
accessList: [AccessListEntry]? = nil, v: BigUInt = 1, r: BigUInt = 0, s: BigUInt = 0) {
// FIXME: This is duplication and should be fixed.
self.data = data
self.accessList = accessList
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know this is WIP but commenting what I see just to make sure. I guess accessList can be removed as well here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also the comments of the function could be cleaned up. As most params are optional but a lot of them still read as (required)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the input.
Next week I'll come back to this PR and will thoroughly check everything you've suggested.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done. Sorry for delay.
A new EIP2930Compatible protocol was introduced to support kind of easy access to accessList.

@salihcnkhy
Copy link

Hi, Is it still testing? When it will be merged to develop? I have a problem with sending parameters to contract methods.

@yaroslavyaroslav
Copy link
Collaborator

@salihcnkhy yep, it still is. About ETA we probably better to ask @JeneaVranceanu, is there much to do here yet?

@JeneaVranceanu
Copy link
Collaborator Author

JeneaVranceanu commented Nov 7, 2022

@salihcnkhy @yaroslavyaroslav Hello there.
I'll try to get back to it tomorrow after 4-5PM GMT+3.
Expect it to be complete tomorrow.

@salihcnkhy
Copy link

@JeneaVranceanu Okay thanks a lot.

@JeneaVranceanu JeneaVranceanu changed the title [WIP] fix: data field in CodableTransaction must be passed into the envelope fix: data field in CodableTransaction must be passed into the envelope Nov 10, 2022
@JeneaVranceanu
Copy link
Collaborator Author

@salihcnkhy @yaroslavyaroslav Sorry for delays. Now, this PR is up to date.

@yaroslavyaroslav yaroslavyaroslav merged commit 410e343 into web3swift-team:develop Nov 10, 2022
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