-
Notifications
You must be signed in to change notification settings - Fork 53
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: added cryptoTransfer to CryptoAllowance to showcase support in HIP906 #800
Conversation
Test Results 17 files + 3 86 suites +16 7m 54s ⏱️ + 1m 18s For more details on these failures, see this check. Results for commit 18f0900. ± Comparison against base commit 024064d. ♻️ This comment has been updated with latest results. |
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.
overall looks good, leaved some nits and comments.
Also, I would like to see the test cases defined as:
- Grant Allowance (approve) from EOA to EOA
- Grant Allowance (approve) from EOA to Contract
- Grant Allowance (approve) from Contract to EOA
- Grant Allowance (approve) from Contract to Contract
- Get Current Allowance from owner account to another Account
- Get Current Allowance from owner account to an Smart Contract
I also think that the correct resolution of these scenarios will need for the EOA to call the system contract directly. due to Security model limitations.
Finally, not sure why are we adding this examples and tests as part of the HTS-Precompile folder, since is not related to HTS transfers but I would place it in its own folder called HAS-SystemContract (Hedera Account Service System Contract)
contracts/hts-precompile/examples/crypto-allowance/cryptoAllowance.sol
Outdated
Show resolved
Hide resolved
expect(logs.args[2]).to.eq(amount); | ||
}); | ||
|
||
it('Should NOT allow an approval on behalf of hbar owner WINTHOUT its signature', async () => { |
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.
nit: WINTHOUT -> WITHOUT
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.
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.
LGTM. tyvm!
|
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.
LGTM
33aba67
to
2c264cd
Compare
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.
LGTM
Signed-off-by: Logan Nguyen <[email protected]>
Signed-off-by: Logan Nguyen <[email protected]>
Signed-off-by: Logan Nguyen <[email protected]>
Signed-off-by: Logan Nguyen <[email protected]>
…HIP906 Signed-off-by: Logan Nguyen <[email protected]>
…ar allowance to another contract Signed-off-by: Logan Nguyen <[email protected]>
Signed-off-by: Logan Nguyen <[email protected]>
Signed-off-by: Logan Nguyen <[email protected]>
Signed-off-by: Logan Nguyen <[email protected]>
Signed-off-by: Logan Nguyen <[email protected]>
Signed-off-by: Logan Nguyen <[email protected]> Co-authored-by: lukelee-sl <[email protected]> Signed-off-by: Logan Nguyen <[email protected]>
2c264cd
to
18f0900
Compare
…HIP906 (hashgraph#800) * feat: added CryptoAllowance wrapper example contract Signed-off-by: Logan Nguyen <[email protected]> * test: added test coverage for CryptoAllowance Signed-off-by: Logan Nguyen <[email protected]> * fix: renamed IHRC to IHRC719 Signed-off-by: Logan Nguyen <[email protected]> * feat: added IHRC632 Signed-off-by: Logan Nguyen <[email protected]> * feat: added cyprtoTransfer to CryptoAllowance to showcase support in HIP906 Signed-off-by: Logan Nguyen <[email protected]> * feat: added use case to showcase the support for contract granting hbar allowance to another contract Signed-off-by: Logan Nguyen <[email protected]> * ci: added CryptoAllowance to PublishResults Signed-off-by: Logan Nguyen <[email protected]> * fix: added return statements for hbarAllowancePublic Signed-off-by: Logan Nguyen <[email protected]> * fix: stored SC addresses into constants Signed-off-by: Logan Nguyen <[email protected]> * fix: fixed typo Signed-off-by: Logan Nguyen <[email protected]> * fix: fixed typo Signed-off-by: Logan Nguyen <[email protected]> Co-authored-by: lukelee-sl <[email protected]> Signed-off-by: Logan Nguyen <[email protected]> --------- Signed-off-by: Logan Nguyen <[email protected]> Signed-off-by: Logan Nguyen <[email protected]> Co-authored-by: lukelee-sl <[email protected]>
Description:
walletAIHrc632 = new Contract(walletA.address, IHRC632, walletA)
;. This will make it possible for the EOA to directly make calls to the HAS precompile contracts, hence making the EOA top level caller of the transaction without any help from a third party like another SC account. That's when Hip-906 kicks in and allow hbar owner, top level caller, grant an hbar allowance to a spender.Related issue(s):
Fixes #797
Notes for reviewer:
Checklist