-
Notifications
You must be signed in to change notification settings - Fork 0
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
[Feature] Billing Contract Integration #127
base: main
Are you sure you want to change the base?
Conversation
Added extension BillingChecker to check if an app has paid or not.
|
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.
Incredible! This is fantastic.
I've spotted a potential roadblock moving forward related to how the globals contract was deployed. I'm gonna create another issue to tackle that. For now if we can keep a copy of the globals.sol contract was without changes i'd be happy to merge.
Some potential moving of files as well happy to discuss
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.
contracts/scripts/core/Globals.s.sol
Line 9 in 20acf66
// TODO: deploy behind a upgradeable proxy see solidstate-solidity/contracts/proxy/upgradeable/UpgradeableProxyOwnable.sol |
Sorry for not noticing this before. We might run into an issue here as globals, from what I can see, was not deployed behind an upgradable proxy. This means we can't upgrade it and if we deploy a new one will mean we have to manually set globals on the apps some how.
Here is my current thinking:
There is another approach we can take by hardcoding the address into the BillingCheckerInternal.sol
but this is going to cause problems with having to manually update that address when testing and deploying which is bound to go wrong at some point. So I don't think it is worth pursuing further.
// SPDX-License-Identifier: BUSL-1.1
pragma solidity ^0.8.16;
import {Global} from "../global/Global.sol";
import {IBillingChecker} from "./IBillingChecker.sol";
import {IBilling} from "../../billing/IBilling.sol";
// hardcoded billing conrtact address
address constant billingContract = 0x123...;
abstract contract BillingCheckerInternal is IBillingChecker, Global {
/**
* @dev wrapper that calls hasPaid from billing contract configured in Globals contract
*/
function _hasPaid(address _app) internal view returns (bool) {
if ( billingContract == address(0) ) {
return false;
}
return IBilling(billingContract).hasPaid(_app);
}
}
I think what you have is the best approach but we need to find a way to redeploy Globals.sol
behind an upgradable proxy and migrate all existing apps over. In theory we could have a facet that gives our open formats wallet privileges to set the global address on all apps. And we create a script to upload that facet. update the globals address on all apps then remove the facet.
For now can we add an OldGlobals.sol contract without these changes, so we can write up and test these migration scripts later.
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.
Wrote up a new issue here to address this https://linear.app/openformat/issue/OF-132/deploy-and-migrate-to-upgradable-globals-contract
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.
I am wondering because these don't follow the diamond storage pattern, should we move this into billing/governed/Governed.sol
The reason being is that if a contract inheriting this uses the diamond storage pattern it will likely break the upgradability because of how variables are stored.
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.
Same here should we move this to src/billing/rescuable/Rescuable.sol
?
Summary
This PR adds a Billing contract that allows collector to create bills for different apps and users to pay those bills.
Description
Billing contract
Current implementation is based on edgeandnode/Billing contract adapted to our case and discussion on OF-49 and OF-54.
This PR does not touch current platform fee mechanism.
Changes:
Governed
andRescuable
extensions for Billing contractBillingChecker
extension that can be used by facets to check whether the app has paid its billsGlobals
contract to get/set Billing contract for all apps-Added Billing contract to deployment process in
Makefile
In order to deploy Billing contract we need to specify several environment variables:
GOVERNOR_ADDRESS
- Address of user that will function as contract governor.COLLECTOR_ADDRESS
- Address of user that will function as initial collector, more collector can be added later.OFT_TOKEN_ADDRESS
- Address of the already deployer ERC20 token that will be used as currency.Related Issue/Bounty
Ref: OF-116
Type of Change
Please check the boxes that apply to your pull request:
Testing
Contract functionalities and integration are tested in
test/billing/Billing.t.sol
for basic functionalitiestest/integration/billing/BillingChecker.sol
forhasPaid
integrationChecklist