-
Notifications
You must be signed in to change notification settings - Fork 44
Create, fund and setup escrow in 1 transaction #3641
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
@portuu3 could you please remove docs changes from this PR? It will be easier to review and also I suppose that we will have to re-generate them before actual release anyway |
53d2220 to
a460e03
Compare
dnechay
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall, have a question though
If my understanding is correct, in place where we want to launch escrow in 1 tx we will have to do something like:
const token = IERC20__factory.connect(tokenAddress, signer);
await token.approve(escrowClient.escrowFactoryContract.target, amount);
const escrowAddress = await escrowClient.createFundAndSetupEscrow(
tokenAddress,
amount,
jobRequesterId,
escrowConfig
);
which means that it will show two pop-ups from the wallet (e.g. Metamask) to the user:
- to approve ERC20 transfer from escrow-factory contract
- to actually create an escrow
I suppose it should be fine for improved UX, because the whole escrow creation process is still once tx that either succeeds or fails. However I'm curious if we could use approach with ERC20Permit or we have limitations/it might be an overhead? So user approves just one tx
@dnechay not all tokens include |
dnechay
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Please double-check why CVAT tests are failing (not sure if it's related to this branch or contacts-v2 in general
It is related to contracts-v2 |
3867c51 to
754143f
Compare
Issue tracking
Closes #3639
Context behind the change
Update escrow factory contract to create, fund and setup escrow in 1 transaction
How has this been tested?
Locally and unit tests
Release plan
Potential risks; What to monitor; Rollback plan
Possible upgrade incompatibilities