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.

Sign up for GitHub

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

Jump to bottom

Make use of Result on completion #221

Merged
merged 1 commit into from
Jun 9, 2022
Merged

Make use of Result on completion #221

merged 1 commit into from
Jun 9, 2022

Conversation

dioKaratzas
Copy link
Collaborator

@dioKaratzas dioKaratzas commented Jun 2, 2022 •
edited
Loading

Its more convenience to use Result to reduce the optionals.

Resolves #219 .
That PR will open the road for the WebSocket integration #182

All the deprecated methods have been putted in extensions with // MARK: - Deprecated for easy removal

@dioKaratzas dioKaratzas requested a review from DarthMike June 2, 2022 17:38
@dioKaratzas dioKaratzas changed the title Make use of Result on completion #219 Make use of Result on completion Jun 2, 2022
@dioKaratzas dioKaratzas added the enhancement New feature or request label Jun 2, 2022
@dioKaratzas dioKaratzas linked an issue Jun 2, 2022 that may be closed by this pull request
Make use of Result on completion #219
Closed
@dioKaratzas
Copy link
Collaborator Author

dioKaratzas commented Jun 2, 2022 •
edited
Loading

Test "testEthSendRawTransaction" fails. My changes don't affect that test. It is failing even on the master. Please take a look on it.

Error failed - Expected tx but failed executionError(Code: -32000 Message: insufficient funds for gas * price + value).

It seems that the test Account is out of money 💵

{Update}
Sent Eth(Faucet) to the ropsten address and the test passed!

@dioKaratzas dioKaratzas mentioned this pull request Jun 2, 2022
Closed
@dioKaratzas dioKaratzas self-assigned this Jun 2, 2022
@dioKaratzas dioKaratzas requested a review from dmcrodrigues June 6, 2022 09:13
@dioKaratzas dioKaratzas mentioned this pull request Jun 7, 2022
Closed
DarthMike
DarthMike reviewed Jun 8, 2022
View reviewed changes
web3swift/src/ENS/EthereumNameService.swift
let resolver = try await getResolver(
for: address,
registryAddress: registryAddress,
mode: mode
)
let resolver = try await getResolver(for: address,
registryAddress: registryAddress,
mode: mode)
Copy link
Member

Choose a reason for hiding this comment

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

are you running formatter on your side? Because it's polluting all meaningful changes for reviewing

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes its formatter, wasn't intended though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@DarthMike I reverted all the unrelated formatted changes.

Copy link
Member

Choose a reason for hiding this comment

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

great 👍

Refactor:
ccb2764 
- EthereumClient
- ThereumClient+Call
- JSONRPC
- EthereumClient+Static
- ERC20

Refactor ERC165

Refactor:

- ERC165
- ERC721
- Multicall

Refactor: EthereumNameService
@DarthMike DarthMike merged commit 2e3087a into argentlabs:develop Jun 9, 2022
@dioKaratzas dioKaratzas deleted the use_result branch June 10, 2022 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DarthMike DarthMike DarthMike approved these changes

@dmcrodrigues dmcrodrigues Awaiting requested review from dmcrodrigues

Assignees

@dioKaratzas dioKaratzas

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make use of Result on completion
2 participants
@dioKaratzas @DarthMike