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

Don't generate native account addresses with Arbitrary #1221

Closed
brson opened this issue Feb 2, 2024 · 0 comments · Fixed by #1240
Closed

Don't generate native account addresses with Arbitrary #1221

brson opened this issue Feb 2, 2024 · 0 comments · Fixed by #1240
Labels
bug Something isn't working

Comments

@brson
Copy link
Contributor

brson commented Feb 2, 2024

What version are you using?

20.2


The Arbitrary impl for Address currently can generate either contract addresses or native addresses, since #1122.

After further experience writing tests with native addresses, I think adding this support was a mistake, as the SDK does not provide the additional tools needed to correctly test using native addresses.

In particular, native addresses need to have storage initialized, trustlines setup, and they cannot sign transactions without additional support code that is not obviously available to testers.

I have written tests where I had to filter out the native addresses because I didn't have those capabilities available.

For now, I think it would be good to just remove the ability to generate native addresses via Arbitrary. This might be considered a regression, but I suspect few are using it anyway.


My token fuzzer does have those capabilities but only with a bunch of custom code that is quite fiddly. It does not use the SDKs ability to generate addresses, but instead has a randomly-generated address "generator" (builder) that can set up storage, create trustlines, and sign transactions.

Something similar might be useful in the SDK.

@brson brson added the bug Something isn't working label Feb 2, 2024
github-merge-queue bot pushed a commit that referenced this issue Mar 5, 2024
This reverts commit 6b04bfc.

Closes #1221

### What

In #1122, I changed the
impl of `Arbitrary` for `Address` to generate all types of addresses
instead of only contract addresses. I no longer think that was the
correct thing to do, so this changes the impl back to only generate
contract addresses.

### Why

Described in the issue
#1221

In short, native addresses are difficult to test with, particularly in
conjunction with the native token contract, because they require set up
of storage and trustlines. Doing this setup is possible, but is not
obvious, and users running into such errors during testing are likely to
get stuck.

### Known limitations

This is arguably a breaking change, though only to the testing
environment, and only to fuzzers. I think it is likely there are so few
fuzzing users that nobody will notice; better to do it sooner than
later.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants
@brson and others