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 an IHRC token facade to support associations via HTS system contract #5950

Closed
6 tasks done
Nana-EC opened this issue Apr 3, 2023 · 15 comments
Closed
6 tasks done
Assignees
Labels
P1 High priority issue, which must be completed in the milestone otherwise the release is at risk.

Comments

@Nana-EC
Copy link
Contributor

Nana-EC commented Apr 3, 2023

Problem

With the removal of delegate calls to system contracts there's no longer a way for an EOA to issue an associate / dissociate transaction in smart contracts.

Solution

Token facades similar to the IERC(tokenAddress).approve provide secure and conformant access to system contract functions.
associate can be exposed in a similar strategy as IERC(tokenAddress).approve.

As such we should

  • Add a IHRC facade design doc
  • Add IHRC facade support in an independent path from current IERC facade
  • Add support for IHRC(tokenAddress).associate() to support an EOA associating with a token
  • Add support for IHRC(tokenAddress).dissociate() to support an EOA associating with a token
  • Add tests in an independent path from current IERC facade testing
  • Add isAssociated() support

Alternatives

No response

@Nana-EC Nana-EC added the P1 High priority issue, which must be completed in the milestone otherwise the release is at risk. label Apr 3, 2023
@mshakeg
Copy link

mshakeg commented May 29, 2023

Hey @Nana-EC I tried attempting an IHRC(tokenAddress).associate() for an ECDSA account on testnet(v0.38.1) and the relay returned the following INVALID_SIGNATURE precheck. I've attached a screenshot of the error message in Metamask.

metamask hashio response

@mshakeg
Copy link

mshakeg commented May 30, 2023

Hey @lukelee-sl could you please confirm if the issue described here is known? As far as I can tell it isn't in which case its a bug.

@lukelee-sl
Copy link
Member

Hey @mshakeg - looking at the logs, it looks to me like that particular transaction was trying to perform a eth_getTransactionCount when it failed which doesn't seem to be directly related to performing the associate. Is this reproducible. Do you have any sample code I could take a look at?


[2023-05-29 17:07:07.671 +0000] [31mERROR[39m (rpc-server/55 on testnet-hashio-547545ff64-9l4bv): [36m[Request ID: 9720115a-57e2-4dd8-b6e1-426256d1b876] [POST] eth_getTransactionCount: 500 (-32603) (JSON RPC ERROR) 145 ms[39m | 2023-05-29T17:07:07.926681924Z
-- | --
[2023-05-29 17:07:07.671 +0000] [31mERROR[39m (rpc-server/55 on testnet-hashio-547545ff64-9l4bv): [36m[Request ID: 9720115a-57e2-4dd8-b6e1-426256d1b876] returning error to sender: Error invoking RPC: transaction [email protected] failed precheck with status INVALID_SIGNATURE[39m | 2023-05-29T17:07:07.926673699Z
[2023-05-29 17:07:07.671 +0000] [32mINFO[39m (rpc-server/55 on testnet-hashio-547545ff64-9l4bv): [36m[Request ID: 9720115a-57e2-4dd8-b6e1-426256d1b876] [POST] eth_getTransactionCount: -32603 145 ms [39m | 2023-05-29T17:07:07.926665877Z
[2023-05-29 17:07:07.671 +0000] [31mERROR[39m (relay-eth/55 on testnet-hashio-547545ff64-9l4bv): [36m[Request ID: 9720115a-57e2-4dd8-b6e1-426256d1b876] Error raised during getTransactionCount for address 0xa16e3686d5e12803b6e30c783be10876dceb8fc9, block number or tag 0x518666[39m | 2023-05-29T17:07:07.926515899Z
[2023-05-29 17:07:07.671 +0000] [90mTRACE[39m (consensus-node/55 on testnet-hashio-547545ff64-9l4bv): [36m[Request ID: 9720115a-57e2-4dd8-b6e1-426256d1b876] [email protected] eth_getTransactionCount AccountInfoQuery status: INVALID_SIGNATURE (7), cost: 21 tℏ[39m | 2023-05-29T17:07:07.926508022Z
[2023-05-29 17:07:07.513 +0000] [90mTRACE[39m (relay-eth/55 on testnet-hashio-547545ff64-9l4bv): [36m[Request ID: 9720115a-57e2-4dd8-b6e1-426256d1b876] getTransactionCount(address=0xa16e3686d5e12803b6e30c783be10876dceb8fc9, blockNumOrTag=0x518666)[39m | 2023-05-29T17:07:07.926384331Z
[2023-05-29 17:07:07.513 +0000] [34mDEBUG[39m (rpc-server/55 on testnet-hashio-547545ff64-9l4bv): [36m[Request ID: 9720115a-57e2-4dd8-b6e1-426256d1b876] eth_getTransactionCount[39m | 2023-05-29T17:07:07.926164281Z



@mshakeg
Copy link

mshakeg commented May 30, 2023

@lukelee-sl thanks for the reply, yes, I just created a script to reproduce it here, if you run the test:tf:testnet script in ./tools/hardhat-example you should observe an error similar to:

[Request ID: 1d0d1481-ab67-4fd2-bf58-a0675a38d49b] Error invoking RPC: transaction [email protected] failed precheck with status INVALID_SIGNATURE

@mshakeg
Copy link

mshakeg commented May 31, 2023

Hey @lukelee-sl I just wanted to check back with you on the above issue.

I also wanted to ask if an isAssociated function could be added to the IHRC facade and perhaps even the HTS precompiled contract as at least in my understanding the IHRC facade functions are only callable directly by an EOA, and not within a function call i.e. call depth > 0, hence it would have to also be added to the HTS precompile for contracts to make this call as well. Though ideally, the IHRC facade should also be callable at call depth > 0.

// for IHRC facade
function isAssociated(address account) view external returns (bool associated);

// for IHederaTokenService
function isAssociated(address token, address account) view external returns (bool associated);

@AlfredoG87
Copy link

AlfredoG87 commented May 31, 2023

@lukelee-sl @mshakeg

Yesterday we were able to identify and fix the problem on testnet that was causing the following error:

[2023-05-29 17:07:07.671 +0000] [90mTRACE[39m (consensus-node/55 on testnet-hashio-547545ff64-9l4bv): [36m[Request ID: 9720115a-57e2-4dd8-b6e1-426256d1b876] [email protected] eth_getTransactionCount AccountInfoQuery status: INVALID_SIGNATURE (7), cost: 21 tℏ[39m | 2023-05-29T17:07:07.926508022Z

Could you verify again and see if now you are able to complete these steps?

@lukelee-sl
Copy link
Member

Hi @mshakeg, to answer your other question regarding isAssociated - I think that's a good suggestion. I need to follow the formal HIP process to get this out and I think the best approach is to update HIP 719 since it is part of the same functionality.

The IHRC facade calls are callable from within a function call - for example here.

contract HRCContract {
    function associate(address token) public returns (uint256 responseCode) {
        return IHRC(token).associate();
    }

    function dissociate(address token) public returns (uint256 responseCode) {
        return IHRC(token).dissociate();
    }
}   

The restriction is that the address being associated to has to be the caller (either the EOA or the contract). Will that be sufficient for your needs for the new isAssociated call?

To clarify I am proposing that we add this call as the token address and the sender are implied.

// for IHRC facade
function isAssociated() view external returns (bool associated);

@mshakeg
Copy link

mshakeg commented May 31, 2023

@AlfredoG87 thanks, it works now, however there seems to be an issue running the test:tf:testnet script against the hashio testnet relay, as it frequently fails with the following response. This is also the case on the Arkhia relay so I don't think it's related to rate limiting.

<html><head>
<meta http-equiv="content-type" content="text/html;charset=utf-8">
<title>502 Server Error</title>
</head>
<body text=#000000 bgcolor=#ffffff>
<h1>Error: Server Error</h1>
<h2>The server encountered a temporary error and could not complete your request.<p>Please try again in 30 seconds.</h2>
<h2></h2>
</body></html>

@mshakeg
Copy link

mshakeg commented May 31, 2023

@lukelee-sl awesome, I don't think it'll be an issue to add IHRC.isAssociated(address account) as that's extending the IHRC facade outlined in HIP-719, however, IHederaTokenService.isAssociated(address token, address account) should not be added as it does not, but also this wouldn't be needed since as you clarified the facade is callable from within a call, and so there's no need to extend IHederaTokenService with this function.

@mshakeg
Copy link

mshakeg commented Jun 1, 2023

Hey @AlfredoG87 I just wanted to ask if y'all have made progress with this issue?

@AlfredoG87
Copy link

Hey @AlfredoG87 I just wanted to ask if y'all have made progress with this issue?

@mshakeg I guess you are referring to the 502 I have not made progress yet, sometimes it is due to high traffic on testnet... but will continue to dig further to see we can optimize it to resolve it and let you know.

@mshakeg
Copy link

mshakeg commented Jun 2, 2023

Hey @AlfredoG87 yes, I'm referring to the 502. I doubt it's a traffic issue as I've very frequently experienced it consistently over the past few days(even with contract deploys) and also the fact that the testnet tps(at least on hederatxns.com) was <10 and the transactions flowing through the hashio testnet relay was likely a small fraction of that(probably at most 1tps).

@AlfredoG87
Copy link

@mshakeg Thank you for the insights, they are very valuable in finding and fixing the issue.
Should we open another ticket for this issue? I feel is unrelated to this one.

@mshakeg
Copy link

mshakeg commented Jun 2, 2023

@AlfredoG87 agreed I'm AFK atm, so I'd appreciate it if you do. Though it seems highly related to hashgraph/hedera-json-rpc-relay/issues/1139

@AlfredoG87
Copy link

AlfredoG87 commented Jun 2, 2023

@mshakeg I have created the ticket so we can tracked it independently of this one.
hashgraph/hedera-json-rpc-relay#1323

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 High priority issue, which must be completed in the milestone otherwise the release is at risk.
Projects
Archived in project
Development

No branches or pull requests

5 participants