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

Decode uint128 & uint64 #177

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

KeKs0r
Copy link
Contributor

@KeKs0r KeKs0r commented Dec 6, 2022

Adding Decoding for uint128 and uint64.

@dawsbot
Copy link
Owner

dawsbot commented Dec 8, 2022

Hi @KeKs0r , thanks for the PR! Can you provide a test which failed before but succeeds now?

@KeKs0r
Copy link
Contributor Author

KeKs0r commented Dec 8, 2022

Checking the other tests, those are quite e2e-ish. So directly against chain. And none of the currently included ABIs have those fields. Should I just add a new ABI that I know uses those fields?

@dawsbot
Copy link
Owner

dawsbot commented Dec 8, 2022

Checking the other tests, those are quite e2e-ish. So directly against chain. And none of the currently included ABIs have those fields. Should I just add a new ABI that I know uses those fields?

Sure, you're right that I did not design the tests well previously because they are very e2e. If you can rip out and make a focused test with an ABI that includes uint128 & uint64 that would be epic!

@dawsbot
Copy link
Owner

dawsbot commented Jan 21, 2023

Hey @KeKs0r I forgot to follow-up here. Was there anything more you needed to get unblocked writing those tests?

Thanks for the PR 🙏

@dawsbot dawsbot closed this Jan 21, 2023
@dawsbot dawsbot reopened this Jan 21, 2023
@dawsbot
Copy link
Owner

dawsbot commented Mar 14, 2023

Checking the other tests, those are quite e2e-ish. So directly against chain. And none of the currently included ABIs have those fields. Should I just add a new ABI that I know uses those fields?

You're spot on, these tests I've written are not using proper mocking nor proper separation of concerns. I'm doing at-least one step to improve this now with #201

@dawsbot
Copy link
Owner

dawsbot commented Apr 24, 2023

I think we're almost ready to solve this.

We can make a test which validates we match encodeFunctionData from ethers: https://docs.ethers.org/v5/api/utils/abi/interface/#Interface--encoding

We currently have this logic all crammed into the other functions but this task should also separate it properly

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.

2 participants