Skip to content

Conversation

JeneaVranceanu
Copy link
Collaborator

@JeneaVranceanu JeneaVranceanu commented Nov 26, 2022

  • Enable writeToChain function to send signed or unsigned transactions;
  • decodeReturnData from ABI.Element.Function returns ["_success": true] instead of nil if a function has no output (WARNING: subject to change)
  • Split decodeReturnData into decodeReturnData and decodeErrorResponse. decodeErrorResponse is able to decode results of revert and require Solidity calls.
  • Tests for decodeReturnData and decodeErrorResponse returning error information caused by revert or require.

@@ -325,7 +325,7 @@ extension ABI.Element.Function {
// set a flag to detect the request succeeded
}

if returnArray.isEmpty {
if returnArray.isEmpty && !outputs.isEmpty {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

While trying to understand what is the issue this line change fixes I understood that we do not have clear explanation for this function outputs.
It's not clear if this change won't just break someone's current use of this library. It's a weird edge case but someone could just be checking if returned data != nil. Thus, we shall clearly specify what to expect in each return case.
What I see so far with current change:

  • nil is returned if something has failed (big minus to us that we don't tell yet what the issue was; must be improved);
  • ["_success": true] for cases when ABI.Element.Function has no return types specified;
  • ["_success": true, ....] for cases when some return types have been specified and data successfully decoded.

Copy link
Collaborator

Choose a reason for hiding this comment

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

pharms-eth gave an example here: #670 (comment)

Copy link
Collaborator Author

@JeneaVranceanu JeneaVranceanu Nov 26, 2022

Choose a reason for hiding this comment

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

I've almost prepared documentation for this function but had to unpack each path leading to return.
It looks like there could be some issues in the function itself when it comes to decoding the invocation of require statement in https://github.com/web3swift-team/web3swift/pull/685/files#diff-3c274f0e8aa4f7a52ab734f9d55c13954e03532655ed01e4ad51747d03fb10e4R304-R312
I'm not sure if it will correctly recognize when it's a result of invocation of empty require() or just 0 bytes returned from a function call.

I want to thoroughly debug and cover it with tests.

/// - `data.count` is less than `outputs.count * 32`;
/// - `outputs` defined and `data` is empty;
/// - `data` represent reverted transaction
public func decodeReturnData(_ data: Data) -> [String: Any] {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What should we also consider here is the option to pass a list or a dictionary of ABI.Element.EthError.
The reason for that is the ability to use custom errors with revert, e.g. revert Unauthorized().
That list must be optional so that we don't force anyone to set it in order to decode data but as a result of having that list we can decode custom errors that were raised using revert because the first 4 bytes of the given data will match function selector of a custom error.

Copy link
Collaborator Author

@JeneaVranceanu JeneaVranceanu Nov 29, 2022

Choose a reason for hiding this comment

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

revert test cases:

revert() -> 0 bytes. We can only assume it was a result of `revert()` or `require(expression)` call. 

revert Unauthorized() -> 0x82b4290000000000000000000000000000000000000000000000000000000000

revert("Not enough Ether provided.") ->
08c379a0
0000000000000000000000000000000000000000000000000000000000000020
000000000000000000000000000000000000000000000000000000000000001a
4e6f7420656e6f7567682045746865722070726f76696465642e000000000000

revert Unauthorized("Reason") -> 
973d02cb
0000000000000000000000000000000000000000000000000000000000000020
0000000000000000000000000000000000000000000000000000000000000006
526561736f6e0000000000000000000000000000000000000000000000000000

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

require test cases:

require(false) -> 0 bytes. We can only assume it was a result of `revert()` or `require(expression)` call.

require(false, "Not enough Ether provided.") ->
08c379a0
0000000000000000000000000000000000000000000000000000000000000020
000000000000000000000000000000000000000000000000000000000000001a
4e6f7420656e6f7567682045746865722070726f76696465642e000000000000

Copy link
Collaborator Author

@JeneaVranceanu JeneaVranceanu Nov 29, 2022

Choose a reason for hiding this comment

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

require is unable to use custom errors so there's no third test case:

It is currently not possible to use custom errors in combination with require. Please use if (!condition) revert CustomError(); instead.

https://docs.soliditylang.org/en/v0.8.17/control-structures.html?highlight=require()#panic-via-assert-and-error-via-require

@JeneaVranceanu JeneaVranceanu marked this pull request as ready for review December 1, 2022 14:58
@JeneaVranceanu JeneaVranceanu self-assigned this Dec 1, 2022
@JeneaVranceanu JeneaVranceanu added the bug Something isn't working label Dec 1, 2022
@JeneaVranceanu JeneaVranceanu added the enhancement New feature or request label Dec 1, 2022
let name = self.name ?? ""
let parameterType = try ABITypeParser.parseTypeString(self.type)
return ABI.Element.EthError.Input(name: name, type: parameterType)
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ABI.Element.EthError.Input was replaced with ABI.Element.InOut since these are two identical structures.
No need for func parseForError() throws -> ABI.Element.EthError.Input then.

// MARK: Sending Data flow
return try await web3.eth.send(transaction)
guard let transactionData = transaction.encode(for: .transaction) else { throw Web3Error.dataError }
return try await web3.eth.send(raw: transactionData)
}
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@janndriessen @yaroslavyaroslav Nothing to review here. This part was discussed in the previous PR.

@@ -25,7 +25,7 @@ class AdvancedABIv2Tests: LocalTestCase {
deployTx.transaction.from = allAddresses[0]
// MARK: Sending Data flow
let policies = Policies(gasLimitPolicy: .manual(3000000))
let result = try await deployTx.writeToChain(password: "web3swift", policies: policies)
let result = try await deployTx.writeToChain(password: "web3swift", policies: policies, sendRaw: false)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same with all , sendRaw: false updates - already discussed.

janndriessen
janndriessen previously approved these changes Dec 1, 2022
Copy link
Collaborator

@janndriessen janndriessen left a comment

Choose a reason for hiding this comment

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

As always great work. 👍

LGTM! We just have to figure out if the failing remote tests are somehow related.

@JeneaVranceanu
Copy link
Collaborator Author

@janndriessen Please re-approve.
Here is everything that has changed after your approval - b9ac465

Copy link
Collaborator

@janndriessen janndriessen left a comment

Choose a reason for hiding this comment

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

🌟 ✅

@JeneaVranceanu
Copy link
Collaborator Author

@yaroslavyaroslav Your approval was dismissed after the #701 was merged. Please re-approve.

@yaroslavyaroslav
Copy link
Collaborator

@yaroslavyaroslav Your approval was dismissed after the #701 was merged. Please re-approve.

Done. Please merge it when you'll be ready.

@JeneaVranceanu JeneaVranceanu merged commit d7a1012 into develop Dec 10, 2022
@JeneaVranceanu JeneaVranceanu deleted the fix/contract-operations branch December 10, 2022 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants