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

add dispute functionality to python test #269

Merged
merged 7 commits into from
Oct 16, 2021
Merged

Conversation

DariusParvin
Copy link
Contributor

Rather than adding a command directly for disputing, I added two commands store and restore for the customer db so we can be more flexible around which revoked state gets broadcast. e.g.

python3 test-zeekoe.py scenario --channel 1 -v --command_list establish pay store pay restore close
python3 test-zeekoe.py scenario --channel 2 -v --command_list establish store pay pay restore close

We can also test what happens if the customer attempts to make a payment on a revoked state e.g.

python3 test-zeekoe.py scenario --channel 2 -v --command_list establish store pay restore pay

Copy link
Contributor

@marsella marsella left a comment

Choose a reason for hiding this comment

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

I like the store/restore paradigm, good idea.

This will break if we run multiple instances in parallel. The second store will overwrite the first. Can you make the temp path be something like temp_path/<channel_id>/customer.db? This will allow us to test separate channels in parallel, at least.

@DariusParvin
Copy link
Contributor Author

DariusParvin commented Oct 6, 2021

Thanks for your feedback @marsella! I like the idea of storing the customer dbs in channel specific directories so that we can run tests on different channels in parallel.

The only issues is when we restore the state, I think that would interfere with other channels that are being run in parallel.

More generally though, I don't think zeekoe can reliably broadcast on-chain operations concurrently for multiple channels with the same tezos account (discussed in #212). In short, the customer or merchant may run into issues if one of them tries to perform two on-chain operations (e.g. establish on channel-2 and close on channel-1) at the same time.

With the current setup, we can do tests where the customer has multiple channels open with the merchant (making sure to run on-chain operations sequentially). However, I think having a test framework that can run independent tests in parallel would take some thinking to get correct and might be better as a separate issue/PR? If you agree then I'll create a separate issue for that and leave this test as is.

Copy link
Member

@jakinyele jakinyele left a comment

Choose a reason for hiding this comment

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

I agree with @marsella but looks good otherwise!

test-zeekoe.py Outdated Show resolved Hide resolved
@marsella
Copy link
Contributor

marsella commented Oct 7, 2021

Ayo, my concern with using a random temp dir is that the current command loop is stateless -- we'd have to add a way to save the temp dir across the lifetime of a loop. I suggested the channel ID because it's deterministic.

Darius, I don't understand your concern with restore interfering with other parallel channels. As long as each channel has a unique ID, restoring one (from a channel-id-specific directory) should be independent of the stored states of the other.

I had forgotten about the Tezos limitations to posting in parallel, though. Please do make an issue for that (specific to testing). I still have a slight preference to changing the file thing now anyway, but if you don't, just make sure it's noted in the issue.

@marsella marsella dismissed their stale review October 7, 2021 14:56

Updated comment to reflect requested changes.

@jakinyele
Copy link
Member

Ayo, my concern with using a random temp dir is that the current command loop is stateless -- we'd have to add a way to save the temp dir across the lifetime of a loop. I suggested the channel ID because it's deterministic.

Ah, great point. We do want it to be deterministic.

@DariusParvin
Copy link
Contributor Author

@marsella an example of what I mean is let's say the customer has two channels, A and B. A is to test dispute, and B is to test normal operation. A stores the db, then restores the db at a later time just before close. If B had made any payments before restore and the channel is still open, then B will be working with a revoked state, no?

@marsella
Copy link
Contributor

marsella commented Oct 7, 2021

Oh, I see. Then yes, that sounds like something for a future issue. Thanks for clarifying.

@DariusParvin DariusParvin requested a review from jakinyele October 8, 2021 17:27
Copy link
Contributor

@yaymukund yaymukund left a comment

Choose a reason for hiding this comment

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

This looks great. I left a few suggestions on how this code could be more idiomatic. Specifically, I think it's important we avoid using os.system if we don't absolutely need it. In my head, os.system is a bit like 'eval' - very powerful, but most of the time it's overkill. The other stuff is nonblocking.

test-zeekoe.py Outdated Show resolved Hide resolved
test-zeekoe.py Outdated Show resolved Hide resolved
test-zeekoe.py Outdated Show resolved Hide resolved
test-zeekoe.py Outdated Show resolved Hide resolved
test-zeekoe.py Show resolved Hide resolved
@DariusParvin
Copy link
Contributor Author

Thanks a lot for the thorough review! I learned a lot from the feedback. It's ready for re-review @yaymukund

test-zeekoe.py Outdated Show resolved Hide resolved
@DariusParvin DariusParvin merged commit 88480de into main Oct 16, 2021
@DariusParvin DariusParvin deleted the add-dispute-test branch October 16, 2021 00:06
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