Conversation
|
Looks good @marko03kostic Once this PR #405 is merged you might need to solve some merge conflicts. Can you add tests for the PaymasterManager? |
|
|
||
| function deployPaymaster() external { | ||
| address paymasterAddress = address(new Paymaster(address(hub))); | ||
| address paymasterAddress = address(new Paymaster(address(hub), msg.sender)); |
There was a problem hiding this comment.
We can set a paymaster at deployment and add it to validPaymasters and deployedPaymasters. Can we add a function to deactivate/remove them as well? Sounds like a good thing to have in case of compromise or if the owner decides to shut it down
There was a problem hiding this comment.
Btw what do we used deployedPaymasters for? Cannot seem to find the use case for it anywhere
| revert("Sender is not the KnowledgeCollection contract"); | ||
| } | ||
|
|
||
| _transferTokens(knowledgeCollectionAddress, amount); |
There was a problem hiding this comment.
Shouldn't we transfer tokens to the StakingStorage like we are doing in KnowledgeCollection.sol if paymaster is not valid?
| import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; | ||
|
|
||
| contract Paymaster is Ownable(msg.sender) { | ||
| contract Paymaster is Ownable { |
There was a problem hiding this comment.
It would be good to emit events when adding or removing from allowed addresses. Same for funding and withdrawing
There was a problem hiding this comment.
I think we should add the transfer ownership method to Paymaster
| @@ -0,0 +1,432 @@ | |||
| import { SignerWithAddress } from '@nomicfoundation/hardhat-ethers/signers'; | |||
There was a problem hiding this comment.
Couldn't find these tests here:
- insufficient Paymaster balance → expect TooLowBalance.
- verify StakingStorage balance increases after coverCost
2 participants
No description provided.