cabbage test for payment v2 #1578
Conversation
boolafish
force-pushed
the
boolafish/cabbage_payment_v2
branch
3 times, most recently
from
3dee95a
to
0b173c9
Compare
| # Somehow the API is designed to return nil txbytes instead of claiming that as bad request :( | ||
| # So we need to return nil here | ||
| # See the test: "test /transaction.create does not return txbytes when spend owner is not provided" | ||
| case Enum.any?(outputs, &(&1.owner == nil)) do |
There was a problem hiding this comment.
damn...I used to be on the side of using if and now.....
boolafish
force-pushed
the
boolafish/cabbage_payment_v2
branch
from
0b173c9
to
3827e25
Compare
boolafish
changed the base branch from
boolafish/multiple-exit-processors
to
payment_v2_poc
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| defmodule OMG.State.Transaction.Payment.Builder do |
There was a problem hiding this comment.
I wonder if different transaction types will have enough shared behaviours to share the same builder? E.g. same number of inputs and outputs, expect the same parameters, etc.
I'm thinking, if not, it might be better to share just the interface (i.e. function signatures) and not the builder implementation (e.g. implying that v1 and v2's inputs can be mapped with Utxo.position/3 and outputs with %Output{})? Otherwise this will be the scary module that everyone's afraid to edit.
There was a problem hiding this comment.
In the ideal state (of my personal thinking), it will be best if each tx type has its own module. This PR is reusing Transaction.Payment that leads to this awkward need of this builder. If each tx type has its own module then the module would be responsible to have its own function like this new and can be interface or behavior-ized. Two payment module can be refactored to share the code between themselves but that is out of the concern of the interface. And new non payment tx type will have nice interface to follow (if everything goes smooth, I know from experience interface breaks often too 😅).
So...I did spend a bit time on having PaymentV2 module. I think it breaks a bit on child chain and breaks quite a lot on watcher-info. The code was not designed for ALD (which makes sense) so nothing is dispatching via tx type at this moment.
As the refactor seems a bit too huge, I want to have test covering first to give us faster feedback loop and some confidence. Also, want to focus on the POC now and try to figure a list of things to refactor on master branch to make it ALD friendly.
There was a problem hiding this comment.
aight sounds good! Thanks for the detailed info!
There was a problem hiding this comment.
can I assume this as an approval of the PR :p
Tested by hacking the watcher info /transaction.create to generate payment v2 tx instead. And then the tx would be signed and send normally. Tested with the cabbage transaction tests. This commit instead of providing new module for payment, it reuses most of the part instead of the function that generate the payment struct. This commit introduces Payment.Builder to build different payment tx. Probably not the most clean way of handling things but the least change required to allow cabbage test to work.
add tx_type as a param of 'order'. If not passed, default to 1 for backward competibility.
linter, dialyzer, remove branching on tx type when building payment tx.
Simplify the test code and make it easier to read
boolafish
force-pushed
the
boolafish/cabbage_payment_v2
branch
from
0c75687
to
e7db8b8
Compare
* feat: test with payment v2 transfer Tested by hacking the watcher info /transaction.create to generate payment v2 tx instead. And then the tx would be signed and send normally. Tested with the cabbage transaction tests. This commit instead of providing new module for payment, it reuses most of the part instead of the function that generate the payment struct. This commit introduces Payment.Builder to build different payment tx. Probably not the most clean way of handling things but the least change required to allow cabbage test to work. * feat: allow /transaction.create to pass in tx_type add tx_type as a param of 'order'. If not passed, default to 1 for backward competibility. * test: cabbage test for payment v2 transfer * fix: cleanup linter, dialyzer, remove branching on tx type when building payment tx. * refactor: use Alice and Bob instead Simplify the test code and make it easier to read * fix: credo complain single pipe
* feat: test with payment v2 transfer Tested by hacking the watcher info /transaction.create to generate payment v2 tx instead. And then the tx would be signed and send normally. Tested with the cabbage transaction tests. This commit instead of providing new module for payment, it reuses most of the part instead of the function that generate the payment struct. This commit introduces Payment.Builder to build different payment tx. Probably not the most clean way of handling things but the least change required to allow cabbage test to work. * feat: allow /transaction.create to pass in tx_type add tx_type as a param of 'order'. If not passed, default to 1 for backward competibility. * test: cabbage test for payment v2 transfer * fix: cleanup linter, dialyzer, remove branching on tx type when building payment tx. * refactor: use Alice and Bob instead Simplify the test code and make it easier to read * fix: credo complain single pipe
📋 closes https://github.com/omgnetwork/new-tx-type-poc/issues/16
Overview
Though the main goal is to be a bit TDD so have e2e test. However, without making change to existing code base it would not be able to run the test. I did try to make some minimum change to just let it work.
There are separate issue tracking the implementation to make the code looks better.
Exit event testing is out of scope of this PR. It is also waiting on #1573
Changes
/transaction.createnow accepttx_typeas a param (optional).Testing
Add cabbage test. run
mix test test/itest/payment_v2_test.exs