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

Add fungible token contract, via HTS #48

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

markogracin
Copy link
Collaborator

  • due compatibility with Hedera (hashscan), and usage with Hashpack, we went in the HTS direction over standard ERC20.

@markogracin markogracin self-assigned this Feb 13, 2025
Copy link

vercel bot commented Feb 13, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
bidi-staging ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 13, 2025 11:14am

Copy link
Member

@pcaversaccio pcaversaccio left a comment

Choose a reason for hiding this comment

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

I quickly glanced through the contract and left some comments.

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 recommend setting the optimiser settings explicitly:

    version: "0.8.28",
    settings: {
      optimizer: {
        enabled: true,
        runs: 999_999,
      },
      evmVersion: "cancun",
    },

Please note that Hedera is compatible with the cancun hard fork: https://docs.hedera.com/hedera/core-concepts/smart-contracts/deploying-smart-contracts#cancun-hard-fork. You should be aware of the semantic differences of the opcodes used on Hedera; see here: https://docs.hedera.com/hedera/core-concepts/smart-contracts/deploying-smart-contracts#solidity-variables-and-opcodes.

// SPDX-License-Identifier: GPL-3.0
pragma solidity 0.8.28;

import './utils/HederaResponseCodes.sol';
Copy link
Member

Choose a reason for hiding this comment

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

Nit: please use explicit imports, e.g.:

import {HederaResponseCodes} from "./utils/HederaResponseCodes.sol";

contract TokenCreator is ExpiryHelper, HederaTokenService {
address private _tokenAddress;
address private _collateralTokenAddress;
address private _owner;
Copy link
Member

Choose a reason for hiding this comment

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

There is the full logic missing around exposing the owner publicly in the ABI & changing the owner. My recommendation would be to import this contract and inherit from it instead: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable.sol.

/**
* @dev Constructor sets the contract owner and initial fee recipient as the deployer
*/
constructor() {
Copy link
Member

Choose a reason for hiding this comment

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

you could pass the owner address as a configurable parameter in the constructor. As proposed above, use https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable.sol.

*/
constructor() {
_owner = msg.sender;
_feeRecipient = msg.sender;
Copy link
Member

Choose a reason for hiding this comment

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

constructor(address feeRecipient_) {
    ...
    if (feeRecipient_== address(0)) {
        revert FeeReceiptInvalid(address(0));
    }
    _feeRecipient = feeRecipient_;
}

* @notice Sets up a 1% fractional fee with min/max limits
* @notice Contract maintains supply and fee management permissions
*/
function createFungible() external payable returns (address createdTokenAddress) {
Copy link
Member

Choose a reason for hiding this comment

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

I have never seen such a function tbh and I'm not sure if this the right way to do it, it's definitely not the right way on Ethereum lol

Copy link
Member

Choose a reason for hiding this comment

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

Also should only be invoked by the owner?

* @return feeRecipient Address where fees are sent
* @return lockedCollateral Amount of collateral currently locked in contract
*/
function getContractInfo() external view returns (
Copy link
Member

Choose a reason for hiding this comment

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

why not additionally defining for each variable a separate function? You can keep this function and simply invoke the functions in the return statement but many times you are only interested in one or two params, e.g. who is the owner.

* @param token Address of the collateral token
* @return responseCode Response code from the Hedera Token Service
*/
function setCollateralToken(address token) external returns (int) {
Copy link
Member

Choose a reason for hiding this comment

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

should be protected no? Only the owner should be able to do this.

address(this),
amount
);
if (transfer != HederaResponseCodes.SUCCESS) revert("Collateral coin transfer failed");
Copy link
Member

Choose a reason for hiding this comment

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

This contract uses inconsistently require and revert. Can we use everywhere custom errors please?

* @notice Transfers all available balance to fee recipient
* @notice Reverts if no fees are available to collect
*/
function collectFees() external {
Copy link
Member

Choose a reason for hiding this comment

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

we should have another emergency function to save any wrongly sent tokens to the contract.

@markogracin
Copy link
Collaborator Author

I quickly glanced through the contract and left some comments.

Thanks @pcaversaccio, appreciate it! Will take a look this weekend, and get back to re-check.

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