-
Notifications
You must be signed in to change notification settings - Fork 16
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
[AVAX-74] Avalanche - Dynamic Issuance #664
base: dev
Are you sure you want to change the base?
Conversation
* | ||
* @dev This address was chosen arbitrarily. | ||
*/ | ||
address public constant BURNED_FOR_BONDINGCURVE_ADDRESS = |
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.
Maybe create a contract that is empty when creating this contract and assign it to the empty contract address, so nobody can mine for the original address
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.
why not use something like 0xDEAD ?
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.
It can be set as 0xDEAD or zero. I wanted to keep it different than those options just to see the amount of burned tokens by the Funding Manager.
I can update it to 0xDead, if it's more convenient.
* @dev `msg.value` is the amount of native tokens to be deposited. | ||
* @param from The address that will have native tokens deposited for. | ||
*/ | ||
function depositNative(address from) external payable { |
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.
Out of curiosity: Why do this as a separate function call in the bonding curve override, instead of calling it from inside the burn() function in this 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.
This is a payable function because I needed to burn native tokens. I implemented it to comply with the current interfaces: IERC20Issuance_v1
, IRedeemingBondingCurveBase_v1
, and IBondingCurveBase_v1
.
To burn tokens, the user needs to transfer native tokens directly to the contract. The current implementation works with ERC20 tokens and uses the approve/allowance mechanism to transfer tokens after user approval.
In the funding manager, I added a payable sellNative
function, where users send native tokens to be burned. This function calls depositNative
, which sends the tokens to NativeIssuance
, responsible for burning them.
I’m not entirely happy with this approach, but it allowed me to implement it without breaking the existing interfaces or rewriting the burning mechanism from scratch.
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 find the whole topic of adapting our system, which was designed in a way that it only takes in ERC20 tokens, to take native tokens kinda difficult to access. @DogaOztuzun did you have the chance to talk about this to anybody else from the Contract Team?
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.
We didn't have a deep discussion and we agreed on an implementation with no breaking changes on the current system.
We agreed on applying IERC20Issuance to the NativeIssuance in a call.
It's never late for the better implementation, I think.
import {Ownable} from "@oz/access/Ownable.sol"; | ||
import {NativeMinterMock} from "test/utils/mocks/external/NativeMinterMock.sol"; | ||
|
||
contract NativeIssuanceTest is Test { |
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.
It would be great if you could add the BTT structure as a comment above the functions, to better keep track of what may be missing. Check out the Bonding Curve test files for an example. There are extension like bulloak that can help to do it fast
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.
added some comments on tests about what they really tests
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.
The meat of it looks good, left some comments
contract FM_BC_Bancor_Redeeming_VirtualSupply_NativeV1Mock is | ||
FM_BC_Bancor_Redeeming_VirtualSupply_Native_v1 | ||
{ | ||
function call_reserveRatioForBuying() external view returns (uint32) { |
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.
we call these "exposed_" now 😁 Check out the repo CoC: https://docs.google.com/document/d/10jYXETE6ySx1e1yLOSY7Ccfy52Mfee1OJUEqGv-_wSA/edit
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.
updated to exposed_
// assertEq( | ||
// issuanceToken.name(), | ||
// string(abi.encodePacked(NAME)), | ||
// "Name has not been set correctly" | ||
// ); | ||
// assertEq( | ||
// issuanceToken.symbol(), | ||
// string(abi.encodePacked(SYMBOL)), | ||
// "Symbol has not been set correctly" | ||
// ); |
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.
outcommented
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.
uncommented
"./utils/mocks/FM_BC_Bancor_Redeeming_VirtualSupply_NativeV1Mock.sol"; | ||
import {NativeMinterMock} from "test/utils/mocks/external/NativeMinterMock.sol"; | ||
|
||
contract FM_BC_Bancor_Redeeming_VirtualSupply_NativeV1Test is ModuleTest { |
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 as with the native minter, BTT would be great :)
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.
added some comments on tests about what they really tests
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.
The meat of it looks good, left some comments
This pull request introduces the Dynamic Issuance feature, which allows for issuing the Native Token of an Avalanche L1 through the Bonding Curve Funding Manager using the Teleporter infrastructure. The implementation focuses on the integration of native token issuance and the Funding Manager is correctly deployed and registered as a module.
Avalanche, Teleporter and InterChain Token Transfer dependent changes are in another repository and there is a Pull Request needs your review;
avalanche-ws/pull-request
Files Requiring Review:
NativeIssuance_v1.sol:
FM_BC_Bancor_Redeeming_VirtualSupply_Native_v1.sol:
DeployPIML1.s.sol: