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

Test cases from waiting to event driven style #369

Merged
merged 31 commits into from
May 2, 2019
Merged

Conversation

marcelomorgado
Copy link
Contributor

Issue link

#329
#273
#219
#354 (comment)

Auto-close the issue?

Closes #273

Types of changes

Bug fix (non-breaking change that fixes an issue)

Meta (a change that affects future changes to this repo)

Technical debt (a code change that doesn't fix a bug or add a feature but makes something clearer for devs)

Notes

This change should be extended to all test cases of the project?


// Gnosis Safe should execute an open order
// Funding ephemeral account with some ethers to pay for gas
// TODO: ephemeralWallet should broadcast this action
Copy link
Member

Choose a reason for hiding this comment

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

That's an interesting idea - curious for your rationale here

Copy link
Member

Choose a reason for hiding this comment

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

Then it's more like a Tasit app clicking a "fund myself" button?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

Copy link
Member

Choose a reason for hiding this comment

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

I guess if a user in the Tasit app clicked fund myself, it would still work just as well if it deep-linked into Gnosis Safe app and then the Gnosis Safe app broadcast it, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so, that test is using the gnosisOwner to call the gnosisSafe contract. So it's like to not having the ephemeral account as a signer and calling the gnosisSafeApp thru deep-link.

Copy link
Member

Choose a reason for hiding this comment

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

Yep. So depending on the scope of this particular test, either option is a sensible choice.

Maybe add something from this GitHub comment thread to explain that in addition to the current TODO comment in the code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll do that.

pcowgill
pcowgill previously approved these changes May 1, 2019
@pcowgill pcowgill dismissed stale reviews from themself via df47472 May 1, 2019 21:42
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.

Decentraland + Gnosis Safe: Create test cases for a failed flow
2 participants