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

feat: implementing hts crypto transfer method (#195) #219

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

arianejasuwienas
Copy link
Contributor

@arianejasuwienas arianejasuwienas commented Jan 23, 2025

Description:

Allows users to call crypto transfer method on the HTS (0x167) address. Since the method is non-payable only foundry solution / library will support hbar transfers (through the vm cheat codes).

Related issue(s):

Fixes #195

Notes for reviewer:

Checklist

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

@arianejasuwienas arianejasuwienas changed the base branch from 195-crypto-transfer-support to main January 23, 2025 14:13
@arianejasuwienas arianejasuwienas changed the title feat: approval support (#195) feat: crypto transfer (#195) Jan 28, 2025
@arianejasuwienas arianejasuwienas force-pushed the 195-crypto-transfer-support-with-approval branch from 2d7b854 to e609cb3 Compare January 30, 2025 09:09
@arianejasuwienas arianejasuwienas self-assigned this Jan 30, 2025
@arianejasuwienas arianejasuwienas added the feature Enhancing an existing feature driven by business requirements. Typically backwards compatible. label Jan 30, 2025
@arianejasuwienas arianejasuwienas force-pushed the 195-crypto-transfer-support-with-approval branch from e609cb3 to cfc904c Compare January 30, 2025 09:30
@arianejasuwienas arianejasuwienas marked this pull request as ready for review January 30, 2025 10:22
@arianejasuwienas arianejasuwienas requested review from a team as code owners January 30, 2025 10:22
@arianejasuwienas arianejasuwienas changed the title feat: crypto transfer (#195) feat: implementing hts crypto transfer method (#195) Jan 30, 2025
andrewb1269hg
andrewb1269hg previously approved these changes Jan 30, 2025
Copy link

@andrewb1269hg andrewb1269hg left a comment

Choose a reason for hiding this comment

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

Review and approve the .gitignore file.

Copy link

@andrewb1269hg andrewb1269hg left a comment

Choose a reason for hiding this comment

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

Reapprove examples/hardhat-hts-crypto-transfer-hbar/.gitignore

andrewb1269hg
andrewb1269hg previously approved these changes Feb 3, 2025
@@ -0,0 +1,75 @@
// SPDX-License-Identifier: Apache-2.0
Copy link
Contributor

Choose a reason for hiding this comment

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

we need to export this interface (and others like IERC20/IERC721) from contracts/.

Opened a ticket here #227

defaultNetwork: 'testnet',
networks: {
hardhat: {
allowUnlimitedContractSize: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need this?

@@ -0,0 +1,50 @@
/*-
Copy link
Contributor

Choose a reason for hiding this comment

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

we already have a Hardhat example, we should include this test in there.

const hre = require('hardhat');
const { expect } = require('chai');

describe('RPC', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe something like

Suggested change
describe('RPC', function () {
describe('cryptoTransfer', function () {

Comment on lines 42 to 47
testnet: {
url: 'https://testnet.hashio.io/api',
accounts: process.env.TESTNET_OPERATOR_PRIVATE_KEY
? [process.env.TESTNET_OPERATOR_PRIVATE_KEY]
: [],
chainId: 296,
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be setup as forking. Otherwise, it's an internal test, not an example test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it was more of an internal test. This feature (hbar transfer) will not work in the fork when using hardhat, even with our plugin. It relies on the foundry cheatcodes. I'll just remove this example for now.

Copy link
Contributor

@acuarica acuarica left a comment

Choose a reason for hiding this comment

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

looking good, left some thing to iterate on.

@arianejasuwienas arianejasuwienas force-pushed the 195-crypto-transfer-support-with-approval branch from 8bc5217 to 8270fd2 Compare February 11, 2025 12:46
@arianejasuwienas
Copy link
Contributor Author

@acuarica is there anything else we want to focus on in this PR?

@acuarica
Copy link
Contributor

Once we are done with pending PRs, I'm going to take a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Enhancing an existing feature driven by business requirements. Typically backwards compatible.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement the cryptoTransfer method defined in the IHederaTokenService
3 participants