Skip to content
This repository was archived by the owner on Aug 2, 2021. It is now read-only.

swap: fix race condition in tests#1693

Merged
holisticode merged 3 commits intoincentivesfrom
incentives-race
Aug 27, 2019
Merged

swap: fix race condition in tests#1693
holisticode merged 3 commits intoincentivesfrom
incentives-race

Conversation

@holisticode
Copy link
Copy Markdown
Contributor

This PR fixes a race condition in tests.

Due to the asynchronous execution of submitting a cheque and cashing it in immediately, if we call that sequence manually in a test, the function will return before the intended operations have terminated (go routine), affecting the nonce for subsequent calls.

Copy link
Copy Markdown
Member

@ralph-pichler ralph-pichler left a comment

Choose a reason for hiding this comment

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

Seems to work. Not the biggest fan of having yet another function as a global variable but it should be ok for now (the cashing logic needs complete rewriting and sane error handling anyway, maybe we can get rid of this then). Is there a specific reason why WaitFunc is defined on Swap but defaultSubmitChequeAndCash is a global var?

@holisticode
Copy link
Copy Markdown
Contributor Author

@ralph-pichler not sure I get your comment completely, but WaitFunc is not on Swap, but is a global variable of the contracts/swap package.

@ralph-pichler
Copy link
Copy Markdown
Member

@holisticode you're right. Mistook the cswap import alias for an object.

@holisticode
Copy link
Copy Markdown
Contributor Author

I indeed hope that the rewrite of the cashing logic will address this current design in a more profound way

testChequeContract = common.HexToAddress("0x4405415b2B8c9F9aA83E151637B8378dD3bcfEDD")
gasLimit = uint64(8000000)
testBackend *backends.SimulatedBackend
errc chan error
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This channel is just closed, it should be chan struct{}, and probably should be renamed to something like submitDone.

I would also like to see that this channel is not in the global scope if that is even possible. Not crucial.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I removed it from global, although the design is still not fantastic but is acceptable now in view that the whole async submit and cashing flow will be refactored

@holisticode holisticode merged commit 6915f76 into incentives Aug 27, 2019
holisticode added a commit that referenced this pull request Aug 27, 2019
* swap: fix race condition in tests
@acud acud deleted the incentives-race branch August 28, 2019 10:17
@mortelli mortelli mentioned this pull request Aug 28, 2019
6 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants