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

atomic assoc contracts #711

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

Conversation

littletarzan
Copy link
Contributor

@littletarzan littletarzan commented Apr 6, 2023

Description:

Related issue(s):

Fixes #

Notes for reviewer:

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

@netlify
Copy link

netlify bot commented Apr 6, 2023

Deploy Preview for hedera-hips ready!

Name Link
🔨 Latest commit 9d7d37b
🔍 Latest deploy log https://app.netlify.com/sites/hedera-hips/deploys/645e80c5000b7a0008f3bf8a
😎 Deploy Preview https://deploy-preview-711--hedera-hips.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@mgarbs mgarbs requested review from Nana-EC and shemnon April 25, 2023 16:17
Copy link
Contributor

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

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

Interesting idea.
Left comments as questions.


## Motivation

Currently, a smart contract attempting to custody an unassociated token will be immediately rejected by the network. This presents a problem in some decentralized finance (DeFi) applications that use smart contracts to atomically custody tokens. Decentraliztion may be achieved by allowing contracts to remain token agnostic, but also prevent spamming attacks by allowing any user to associate tokens to a DeFi smart contract.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the proposal refer to all smart contract regardless of creation path?
For example those created via HAPI ContractCreate have the option of setting max_automatic_token_associations and updating it in a ContractUpdate.

This might be a good section to address why that path is not sufficient for the proposal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This proposal refers to all smart contracts regardless of creation path. Smart contracts are often intermediaries for many different tokens, so it becomes a challenge to maintain association maps and retain decentralization. Presently, dexes are associating/dissociating tokens using precompiles every time they are used as intermediaries. If Hedera was able to refine association checks such that the ending contract balance must be greater than zero, it would eliminate the need to pay 5 cents more per transaction.

We will address why the max_automatic_token_association is not sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Nana-EC this is now addressed on line 24


## Security Implications

Increased complexity of Hedera Smart Contract Service may lead to vulnerabilities.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you expand on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I meant generally speaking there might be a risk for a contract to accept an unassociated token and carry a balance. As long as there's a firm check at the end of the transaction that unassociated token balance = 0, I don't think there is a security risk.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay. Should be explicit in the HIP so it's clear to all


## Specification

Hedera Smart Contract Service optimistically allows custody of any token for smart contracts, and checks at the end of a contract call that tokens are associated to entities whose balance is changing from zero to nonzero.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this feature be opt-in or opt-out?
In either case if applicable what procedure would a contract creator take?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would like to see this applied to every smart contract. If a contract's token balance changes from zero to nonzero during a contract call transaction, then check for association at the end of the transaction. I would also support opt-in or opt-out if there are use cases that call for it. If an opt-in flag is enabled, and the contract can create another contract using CREATE or CREATE2, then the created contracts should inherit the property from the parent contract.

@mgarbs mgarbs requested a review from Nana-EC May 9, 2023 16:47

## References

None.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend adding references to:

  • EIP-20
  • HTS
  • anything specific to HTS associateToken an dissociateToken
  • anything specific to response codes (e.g. where TOKEN_NOT_ASSOCIATED_TO_ACCOUNT and SUCCESS are defined)

N/A

## Reference Implementation
#### Example :
Copy link
Contributor

Choose a reason for hiding this comment

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

In the code examples below, I'd recommend:

  • explicitly define the input parameters (token and recipient)
    • e.g. can recipient only be an EOA, or could it be any address, including SCs?
    • e.g. can token only be an HTS token, or could it be any token, including ones authored in Solidity?
    • @param treasury should be @param recipient in the function-level comments
  • in lieu of the comment // assumes msg.sender approved an allowance to address(this), include the code for the approval, as it is more explicit
  • In function1, the return value of the 2nd invocation of HederaTokenService.transferToken is saved in respCode, whereas in function2 this does not happen
    • I assume that you wanted to do so in both, and assert that in function1 it was SUCCESS, and in function2 it should be TOKEN_NOT_ASSOCIATED_TO_ACCOUNT or your newly proposed TOKEN_NOT_ASSOCIATED_TO_CONTRACT
    • If so, add that assertion or a comment; plus consider adding function3 which results in the newly proposed response code.

Signed-off-by: Matthew DeLorenzo <[email protected]>
Signed-off-by: Matthew DeLorenzo <[email protected]>
Signed-off-by: Matthew DeLorenzo <[email protected]>

## Abstract

Smart contracts are required to be associated with all tokens they custody. This makes sense when the contract has a long lived balance > 0; Hedera state is expanded and must be paid for in ongoing maintenance and storage costs to the network. However, there are many use cases that call for smart contracts atomically taking custody of a token, often to transfer to another entity within the same contract call. In this case, association is still required but Hedera state is not expanded.
Copy link
Collaborator

Choose a reason for hiding this comment

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

My perspective here is that if the contract ever held custody of an asset, it should pay the association cost out of sheer fairness to all entities in the network. Doing something else is giving special treatment to contracts when they are just another entity and should be treated as such.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@littletarzan want to make sure you saw this comment ^^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mgarbs Is it true the association fee is to help pay for expanded network state size? If so, then atomic custody of tokens by smart contracts without association would not add to the net size of the network state. It seems unreasonable to have to pay the association fee to custody a token for an atomic or infinitesimal amount of time.

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.

4 participants