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

feat: improve decode error #831

Merged

Conversation

zhangliugang
Copy link
Contributor

Summary of Changes

Handle revert exception

When a contract call reverted, we may get different data format of response data from different chains, for example:

{
  "jsonrpc": "2.0",
  "id": 1,
  "result": "0x6ca7b806000000000000000000000000581074d2d9e50913eb37665b07cafa9bffdd1640"
}
{
  "jsonrpc": "2.0",
  "id": 1,
  "error": {
    "code": 3,
    "data": "0x6ca7b8060000000000000000000000009cf91286f22a1b799770fb5de0e66f3c4cc165d1",
    "message": "execution reverted"
  }
}

As so far we can only handle the first condition, So in APIRequest+Method.swift, I add a new function to handle the second one, and return the error.data field as result
Seems ethers.js has a complex logic about this, I am not sure which chain will return a revert error in more deeper nesting object.

RequestBody

RequestBody now accept any object conforms to Encodable as params, maybe we can remove APIRequestParameterType safely.

Break changes

Refactor ContractProtocol.decodeReturnData(_ method: String, data: Data) -> [String: Any]?
to ContractProtocol.decodeReturnData(_ method: String, data: Data) throws -> [String: Any]

This function will throw Web3Error.revert(String, reason: String?) or Web3Error.revertCustom(String, [String: Any]) when data parameter contains an error.
Removed fields with _ prefix from return Dictionary, it only return fields that decoded from given Function.

This also effects ReadOperation.callContractMethod


Refactor ABI.Element.Function.decodeReturnData(_ data: Data, errors: [String: ABI.Element.EthError]? = nil) -> [String: Any]
to ABI.Element.Function.decodeReturnData(_ data: Data) throws -> [String: Any]

Add decode error method to ABI.Element.EthError, error decode is moved from Function to EthError

many test data in ABIElementErrorDecodingTest.swfit has wrong byte length, i have no idea why those tests were passed.

Test Data or Screenshots

By submitting this pull request, you are confirming the following:
  • I have reviewed the Contribution Guidelines.
  • I have performed a self-review of my own code.
  • I have updated my repository to match the develop branch.
  • I have included test data or screenshots that prove my fix is effective or that my feature works.
  • I have checked that all tests work and swiftlint is not throwing any errors/warnings.

@zhangliugang zhangliugang changed the title Feat/decode error feat: decode error Aug 29, 2023
@zhangliugang zhangliugang changed the title feat: decode error feat: improve decode error Aug 29, 2023
@JeneaVranceanu JeneaVranceanu self-requested a review August 29, 2023 06:12
Sources/Web3Core/Contract/ContractProtocol.swift Outdated Show resolved Hide resolved
Sources/Web3Core/EthereumABI/ABIElements.swift Outdated Show resolved Hide resolved
Sources/Web3Core/EthereumABI/ABIElements.swift Outdated Show resolved Hide resolved
return "\(name)(\(inputs.map { $0.type.abiRepresentation }.joined(separator: ",")))"
}

public var methodString: String {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like this could be named selector as this is the terminology used in Solidity.
I see that methodString should be rather something like ErrorXyz(uint256,...) and signature should be 0xcafe1234. The same applies for all other types e.g. Function, Event etc.

For now, no action here is required. I'll take a look at the terminology used in Solidity and then we can think about renaming some of our variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

or leave the signature unchanged, add a new signatureHash for hash, mark methodEncoding and methodString as deprecated

Comment on lines 204 to 206
public var methodEncoding: Data {
return signature.data(using: .ascii)!.sha3(.keccak256)[0...3]
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the methodEncoding? Maybe I'm forgetting something but it looks unusual.
Any links to docs about it would be appreciated!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, you borrowed it from Function, I guess.
Oh, methodEncoding of Function is from 2018. We need documentation for it for sure.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Screenshot 2023-08-29 at 09 59 39

Copy link
Collaborator

Choose a reason for hiding this comment

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

So it's the same just raw bytes instead of a string.

Tests/web3swiftTests/remoteTests/InfuraTests.swift Outdated Show resolved Hide resolved
@yaroslavyaroslav
Copy link
Collaborator

@zhangliugang just in case you didn't know: you're able to run all tests locally since all of them are stored in ./.github/ folder as a plain and pretty straightforward yml configs.

Copy link
Collaborator

@JeneaVranceanu JeneaVranceanu left a comment

Choose a reason for hiding this comment

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

I'll try to answer to all comments and push some changes into this PR to lead it to closure until December 3rd.

@JeneaVranceanu
Copy link
Collaborator

@zhangliugang Please approve this PR zhangliugang#1
Unless you have comments.

Copy link

@Syn-McJ Syn-McJ left a comment

Choose a reason for hiding this comment

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

Looks like a great improvement over error decoding.

@Syn-McJ
Copy link

Syn-McJ commented Jan 4, 2024

Can this be merged? Not having data in the JsonRpcErrorObject is a real pain.

@yaroslavyaroslav
Copy link
Collaborator

yaroslavyaroslav commented Jan 4, 2024

Although I didn't reviewed it appropriately, I'll merge it tomorrow if @JeneaVranceanu has no objections on this.

Copy link
Collaborator

@JeneaVranceanu JeneaVranceanu left a comment

Choose a reason for hiding this comment

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

Double checking in progress.

@yaroslavyaroslav
Copy link
Collaborator

@JeneaVranceanu are we there yet?

@JeneaVranceanu
Copy link
Collaborator

@yaroslavyaroslav Ready to be merged once CI finishes successfully. Will need approval from you because the last commits are authored by me.

@yaroslavyaroslav yaroslavyaroslav merged commit ef28c1f into web3swift-team:develop Jan 9, 2024
2 checks passed
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