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

contract, swap: refactor backend to commit on sendTransaction #2092

Merged
merged 13 commits into from
Feb 12, 2020

Conversation

ralph-pichler
Copy link
Member

@ralph-pichler ralph-pichler commented Feb 6, 2020

This PR introduces the chain.WaitMined function which supersedes the global contract.WaitFunc (which was used for testing and modified by the setupContractTest helper function) and the bind.WaitMined function.

During tests , the simulated backend is wrapped in a chain.TestBackend which commits after a successful SendTransaction. All backend specific stuff was moved from the contracts/swap package to the new swap/chain package which will contain the transaction queue in a follow-up PR.

bind.WaitMined is no longer used as it required a types.Transaction which might not be available. Instead a slightly modified version thereof is included in the chain package. The setupContractTest is only used for the cash cheque function which will be removed in the next PR, afterwards setupContractTest will be dropped.

@ralph-pichler ralph-pichler changed the title contract, swap: refactor backend to include WaitMined contract, swap: refactor backend to commit on sendTransaction Feb 6, 2020
Copy link
Contributor

@Eknir Eknir left a comment

Choose a reason for hiding this comment

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

Functionally equivalent. Refactor by putting the backend logic to a separate file.

From the PR description, it is not immediately clear how and why this modification was needed, but I assume a follow-up PR comes where the purpose of this refactor will become clear.

I added one comment about the Close function which is not implemented. Please address this one before merging.

swap/txqueue/backend.go Outdated Show resolved Hide resolved
swap/txqueue/backend.go Outdated Show resolved Hide resolved
@ralph-pichler
Copy link
Member Author

ralph-pichler commented Feb 7, 2020

@Eknir regarding the why this was needed:

  • the current way of testing by modifying global variables (WaitFunc and cashCheque) is very suboptimal. It needs the setupContractTest helper which adds clutter to tests and can have strange behaviour if used recursively with other helper functions which need this. With this change half of those problems go away (and with the follow-up PR setupContractTest will be retired completely).

  • bind.WaitMined was replaced because it only takes a types.Transaction which is quite difficult to obtain if you didn't send the transaction from the running code (e.g. after a restart). The new function makes tracking transactions by hash much easier.

  • the reason for the package move is that swap/chain or whatever it is going to be called in the end, will be the single component responsible for tracking transactions, therefore it makes sense for things like backend and wait functions to be defined there.

@acud
Copy link
Member

acud commented Feb 10, 2020

swap/chain/backend.go Outdated Show resolved Hide resolved
swap/chain/backend.go Outdated Show resolved Hide resolved
swap/chain/backend.go Show resolved Hide resolved
swap/chain/backend.go Outdated Show resolved Hide resolved
swap/chain/backend.go Outdated Show resolved Hide resolved
Copy link
Member

@acud acud left a comment

Choose a reason for hiding this comment

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

few more minor things but otherwise LGTM

swap/chain/backend.go Outdated Show resolved Hide resolved
swap/chain/backend.go Show resolved Hide resolved
@ralph-pichler ralph-pichler merged commit 052e345 into master Feb 12, 2020
@ralph-pichler ralph-pichler deleted the swap_backend_rewrite branch March 3, 2020 20:36
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.

None yet

4 participants