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

feat: add erc20 paymaster #6

Merged
merged 46 commits into from
Feb 13, 2024
Merged

feat: add erc20 paymaster #6

merged 46 commits into from
Feb 13, 2024

Conversation

aliXsed
Copy link
Collaborator

@aliXsed aliXsed commented Jan 25, 2024

This is a paymaster that allows the gas fee to be paid from our own token. This version relies on a price which can be updated by a permissioned oracle. The admin can easily kick out the oracle at any point and replace her with another trusted wallet. The admin can also withdraw the ETH that this paymaster is charged with.

  • Add unit test
  • Add deploy script

contracts/contracts/paymasters/Erc20Paymaster.sol Outdated Show resolved Hide resolved
contracts/contracts/paymasters/Erc20Paymaster.sol Outdated Show resolved Hide resolved
contracts/contracts/paymasters/Erc20Paymaster.sol Outdated Show resolved Hide resolved
Comment on lines 31 to 33
function updateFeePrice(uint256 _feePrice) public onlyRole(PRICE_ORACLE_ROLE) {
feePrice = _feePrice;
}
Copy link
Member

Choose a reason for hiding this comment

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

I would like to move to a more agnostic architecture where this contract (the Paymaster) pulls the information from another contract which is the price feed itself. Meaning that we have two contracts:

  1. The Paymaster, with a configurable price feed contract (let's say it implements an interface IPriceFeed)
  2. The actual price feed implementing IPriceFeed and exposing this updateFeePrice method for its robot to update the price

This will allow us to easily move to another price feed contract in the future without having to rewrite or redeploy the paymaster

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup that was the next step. As discussed we wanted to have a minimally working contract quickly and improving it towards the ideal case. Uniswap has already got some interfaces that could be used here even if we are not going to connect the contract to uniswap immediately.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think it makes sense to have now

paymaster -> price feed

Then when we have the uniswap (or similar) price feed we can do

paymaster -> price feed -> uniswap

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This PR has already grown too big, if you agree let's not keep it for this feature.

Copy link
Member

Choose a reason for hiding this comment

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

I am fine with this if this PR is followed by one implementing the above. It sounds necessary for production from my perspective as it allows us some flexibility oracle wise in case of upgrades etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes it will be followed

contracts/contracts/paymasters/Erc20Paymaster.sol Outdated Show resolved Hide resolved
Copy link
Member

@ETeissonniere ETeissonniere left a comment

Choose a reason for hiding this comment

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

@aliXsed given the adjustments to use the docker node, we will need to adapt the Github Actions to start the said local node. Careful that it will increase the CI time by a lot since it takes 5 to 10 minutes to completely init a local testnet.

contracts/contracts/NODL.sol Outdated Show resolved Hide resolved
contracts/contracts/NODL.sol Outdated Show resolved Hide resolved
contracts/deploy/utils.ts Outdated Show resolved Hide resolved
contracts/test/paymasters/Erc20Paymaster.test.ts Outdated Show resolved Hide resolved
@aliXsed aliXsed marked this pull request as ready for review February 13, 2024 02:07
Comment on lines +12 to +13
"fmt:check": "prettier --check --plugin=prettier-plugin-solidity 'contracts/**/*.sol' '**/*.ts'",
"fmt:fix": "prettier --write --plugin=prettier-plugin-solidity 'contracts/**/*.sol' '**/*.ts'"
Copy link
Member

Choose a reason for hiding this comment

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

Niiiiice thanks!

@ETeissonniere ETeissonniere merged commit e56dada into main Feb 13, 2024
1 check passed
@ETeissonniere ETeissonniere deleted the aliX/nodl-paymaster branch February 13, 2024 17:41
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.

2 participants