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

Frontend: support subsidized transfers #2031

Merged
merged 10 commits into from
Jul 12, 2023

Conversation

GabrielBuragev
Copy link
Contributor

@GabrielBuragev GabrielBuragev commented Jul 10, 2023

Closes #2026

In order to test it, you need to add a feeSubAddress in the config/chains/421613.json chain config file.
The feeSubAddress property inside the config file will be used as a toggle point for this feature on the frontend.
If the feeSubAddress is not defined in the chain config file, then the app resumes with normal operation (doesn't do any checks agains the FeeSub contract).

I used to test with the following FeeSub contract deployed on Arbitrum: https://goerli.arbiscan.io/address/0xda8015d66c30d6a3d4dd112fd175a9ee3ff38c74

One thing worries me, which was ignored in order to speed up the development process of this feature:
When the FeeSub contract is defined in a chain config file, but it has not enough funds to subsidize a transfer, the transfers created on the frontend will continuously fail until one of the following was done:

  • FeeSub property was removed from the specific chain config file and the frontend is re-deployed
  • Someone funds the FeeSub contract on the specific chain so it can continue subsidizing transfers

We cannot easily prevent this on the frontend since:

  • Contract has no book-keeping for the funds being deposited/used
  • Contract doesn't provide a way to check if it has enough funds to subsidize a specified transfer amount

We could do these checks on our end, but i do believe that this should be done on contract level instead.

Copy link
Contributor

@manuelwedler manuelwedler left a comment

Choose a reason for hiding this comment

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

One issue regarding the decoding from local storage. Other than that, the implementation looks perfect. Just a few more things that came to my mind...

Did we ever think of showing the paid fees in the transfer history?
Imo, before going live, we should definitely be able to switch to use the request manager when the FeeSub contract is drained. I don't mind whether we provide a check on contract-level or on the frontend.

Ah and we should check why the build fails in the CI before merging.

@GabrielBuragev GabrielBuragev force-pushed the frontend/support-subsidized-transfers branch from 2093b4d to 5e17fda Compare July 11, 2023 07:36
@GabrielBuragev GabrielBuragev changed the title Frontend/support subsidized transfers Frontend: support subsidized transfers Jul 11, 2023
@GabrielBuragev
Copy link
Contributor Author

Additionally, tests for the subsidized-transfer.ts were missing. Included them in the last fixup commit

@istankovic
Copy link
Contributor

Did we ever think of showing the paid fees in the transfer history?

Good that you asked that. Today I wanted to see exactly that (the fees) in my transfer history. :-)

@GabrielBuragev
Copy link
Contributor Author

Here you go: #2042

@bilbeyt bilbeyt force-pushed the frontend/support-subsidized-transfers branch from ab063af to 4663f47 Compare July 11, 2023 14:08
frontend/package.json Outdated Show resolved Hide resolved
contracts/contracts/FeeSub.sol Show resolved Hide resolved
frontend/tests/unit/services/transactions/fee-sub.spec.ts Outdated Show resolved Hide resolved
frontend/src/services/transactions/token.ts Outdated Show resolved Hide resolved
frontend/src/actions/transfers/subsidized-transfer.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@manuelwedler manuelwedler left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks for the work!

GabrielBuragev and others added 10 commits July 12, 2023 18:51
Previously, we only needed to generate typings for the
@beamer-bridge/deployments npm package. That changed with the new
`FeeSub` contract. The decision was to not keep this contract as a part
of our deployments folder/npm package so
we have to copy/paste the ABI for this contract
whenever it changes.
Due to this reason, we had to extend the `generate-types`
npm command to detect and use this folder as well.
In order to keep the output folder structure of this command simple,
i used the `typechain` command twice so it doesn't try to come up with
a complex folder structure for the outputted contracts which breaks
the module import resolution in our codebase.
In order to be able to easily determine if a transfer
can be subsidized, we implement
a view function that enables us to do exactly that.
@GabrielBuragev GabrielBuragev force-pushed the frontend/support-subsidized-transfers branch from 19a991b to a339140 Compare July 12, 2023 16:51
@GabrielBuragev GabrielBuragev merged commit b0a97b0 into main Jul 12, 2023
@GabrielBuragev GabrielBuragev deleted the frontend/support-subsidized-transfers branch July 12, 2023 16:54
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.

Frontend: Implement a way to use a subsidizer contract
5 participants