docs: Provide Design document for HIP-904 System Contracts flows #15435

stoyanov-st · 2024-09-11T13:29:30Z

Description:

This PR proposes a design document for the upcoming implementation of the HIP-904 system contracts functions and logic.

Related issue(s):

932
Fixes #

Notes for reviewer:

Checklist

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

Signed-off-by: Stanimir Stoyanov <[email protected]>

codacy-production · 2024-09-11T13:37:52Z

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ -0.03% (target: -1.00%)	✅ ∅

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (`2dc454a`)	108950	67305	61.78%
Head commit (`8eb75b9`)	109271 (+321)	67467 (+162)	61.74% (-0.03%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#15435)	0	0	∅ (not applicable)

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings Change summary preferences

_{Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more}

codecov · 2024-09-11T13:38:07Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 58.21%. Comparing base (2dc454a) to head (8eb75b9).
Report is 33 commits behind head on develop.

Additional details and impacted files

@@              Coverage Diff              @@
##             develop   #15435      +/-   ##
=============================================
- Coverage      58.23%   58.21%   -0.03%     
- Complexity     21582    21617      +35     
=============================================
  Files           2779     2785       +6     
  Lines         109133   109530     +397     
  Branches       11183    11210      +27     
=============================================
+ Hits           63557    63764     +207     
- Misses         41718    41890     +172     
- Partials        3858     3876      +18

see 129 files with indirect coverage changes

...a-node/docs/design/services/smart-contract-service/frictionless-airdrops-system-contracts.md

Signed-off-by: Stanimir Stoyanov <[email protected]>

Nana-EC

1st pass thoughts

...a-node/docs/design/services/smart-contract-service/frictionless-airdrops-system-contracts.md

Nana-EC · 2024-09-17T03:47:22Z

...a-node/docs/design/services/smart-contract-service/frictionless-airdrops-system-contracts.md

+| `0xc1acfe5d`           | `function cancelAirdrops(PendingAirdrop[] memory pendingAirdrops) external returns (int64 responseCode)`                            | `ResponseCode`                           | The response code from the call                                           |
+| `0x241b6af7`           | `function claimAirdrops(PendingAirdrop[] memory pendingAirdrops) external returns (int64 responseCode)`                             | `ResponseCode`                           | The response code from the call                                           |
+| `0x012ea0b1`           | `function rejectTokens(address[] memory ftAddresses,  NftId[] memory nftIds) external returns (int64 responseCode)`                 | `ResponseCode`                           | The response code from the call                                           |
+| `0xc60ffe1b`           | `function getPendingAirdrops(address account) external returns (int64 responseCode, PendingAirdropRecord[] memory pendingAirdrops)` | `ResponseCode`, `PendingAirdropRecord[]` | The response code from the call and the pending airdrops for given sender |


I don't think we should be adding this function in its form.
How would we handle when the number of airdrops is significant? We can' sent it all and if you send a partial amount how does a user get this information

I see your point. This one is a proposal, as we only have the Mirror node to query for the Pending Airdrops where we have pagination. Maybe we should skip the query part and leave it to the Mirror node for now?

I wonder what the use case is for a smart contract to call this (as opposed to a wallet or something else external to the blockchain) calling the mirror node)?

Signed-off-by: Stanimir Stoyanov <[email protected]>

david-bakin-sl

Some suggestions.

david-bakin-sl · 2024-09-17T15:34:42Z

...a-node/docs/design/services/smart-contract-service/frictionless-airdrops-system-contracts.md

+    address receiver;
+
+    address token;
+    bool isNft;


Why not an enum for (fungible, non-fungible) instead of this bool isNft, then put it above the address token. Reason would be to look more similar to the protobuf PendingAirdrop.

david-bakin-sl · 2024-09-17T15:35:48Z

...a-node/docs/design/services/smart-contract-service/frictionless-airdrops-system-contracts.md

+| `0xc1acfe5d`           | `function cancelAirdrops(PendingAirdrop[] memory pendingAirdrops) external returns (int64 responseCode)`            | `ResponseCode` | The response code from the call |
+| `0x241b6af7`           | `function claimAirdrops(PendingAirdrop[] memory pendingAirdrops) external returns (int64 responseCode)`             | `ResponseCode` | The response code from the call |


Here (and in next table) the HIP limits the number of pending airdrops to a maximum of 10. That limit should be in this document somewhere - perhaps in this table, perhaps as text following the table? (Also reject.)

(Reason to have it in this doc: reminder to test the limits.)

david-bakin-sl · 2024-09-17T15:45:08Z

...a-node/docs/design/services/smart-contract-service/frictionless-airdrops-system-contracts.md

+}
+```
+
+`NftId` - A struct that represents the Nft serials to be rejected.


Also is NftId the right name - there are multiple Nfts you can identify by their serial. So, plural vs singular? Doesn't really match the HIP either which has a different defintiion of NftId in the protobuf - a singular tokentype+serial number.

Yeah, this should match the proto or have a different name. Should be 1 token type (address in our case) and 1 serial.
To do multiple Nfts you'd do an array of these

david-bakin-sl · 2024-09-17T15:56:27Z

...a-node/docs/design/services/smart-contract-service/frictionless-airdrops-system-contracts.md

+- Verify that the `airdropTokens` function fails when the token does not exist.
+- Verify that the `airdropTokens` function fails when the airdrop amounts are out of bounds.
+- Verify that the `cancelAirdrops` function fails when the sender does not have any pending airdrops.
+- Verify that the `cancelAirdrops` function fails when the sender does not have a valid account.


Here for example is where you'd test you can't have more than 10 listed. And above that you can have 10 (in the positive tests).

Now, there's a question as to whether we need to test this as the underlying HAPI call will return a response code indicating the error. ????

Actually it occurs to me you could check the static limit of 10 during precheck, fail very early. If you do this it (and other precheck checks) should be called out in this design doc.

david-bakin-sl · 2024-09-17T16:02:28Z

...a-node/docs/design/services/smart-contract-service/frictionless-airdrops-system-contracts.md

+| `0x63ada5d7`           | `function claimAirdropNFT(address senderAddress, int64 serialNumber) external returns (uint256 responseCode)`    | `ResponseCode` | The response code from the call |
+| `0x76c6b391`           | `function rejectTokenFT() external returns (uint256 responseCode)`                                               | `ResponseCode` | The response code from the call |
+| `0xa869c78a`           | `function rejectTokenNFTs(int64[] memory serialNumbers) external returns (uint256 responseCode)`                 | `ResponseCode` | The response code from the call |
+| `0x966884d4`           | `function setAutomaticAssociations(int64 newAutoAssociations) external returns (uint256 responseCode)`           | `ResponseCode` | The response code from the call |


This is HAS not HTS right? Does that need to be called out?

Signed-off-by: Stanimir Stoyanov <[email protected]>

lukelee-sl · 2024-09-17T21:37:21Z

I pretty much agree with the comments by @Nana-EC and @david-bakin-sl. Nothing to add from me.

Nana-EC

Good improvements.
3 remaining suggestions

Nana-EC · 2024-09-18T17:53:27Z

...a-node/docs/design/services/smart-contract-service/frictionless-airdrops-system-contracts.md

+}
+```
+
+`NftId` - A struct that represents the Nft serials to be rejected.


Yeah, this should match the proto or have a different name. Should be 1 token type (address in our case) and 1 serial.
To do multiple Nfts you'd do an array of these

...a-node/docs/design/services/smart-contract-service/frictionless-airdrops-system-contracts.md

Signed-off-by: Stanimir Stoyanov <[email protected]>

doc: design doc for hip-904 system contracts

3a9eecc

Signed-off-by: Stanimir Stoyanov <[email protected]>

stoyanov-st added the Hedera Smart Contract Service Issues related to the Hedera Smart Contract Service. label Sep 11, 2024

stoyanov-st added this to the v0.55 milestone Sep 11, 2024

stoyanov-st self-assigned this Sep 11, 2024

stoyanov-st linked an issue Sep 11, 2024 that may be closed by this pull request

[R&D] HIP-904 Fricitonless Airdrops System Contract Functions hashgraph/hedera-smart-contracts#932

Open

stoyanov-st changed the title ~~doc: Provide Design document for HIP-904 System Contracts flows~~ docs: Provide Design document for HIP-904 System Contracts flows Sep 11, 2024

stoyanov-st requested a review from a team September 11, 2024 13:34

lukelee-sl reviewed Sep 12, 2024

View reviewed changes

...a-node/docs/design/services/smart-contract-service/frictionless-airdrops-system-contracts.md Show resolved Hide resolved

lukelee-sl reviewed Sep 12, 2024

View reviewed changes

...a-node/docs/design/services/smart-contract-service/frictionless-airdrops-system-contracts.md Show resolved Hide resolved

lukelee-sl reviewed Sep 12, 2024

View reviewed changes

...a-node/docs/design/services/smart-contract-service/frictionless-airdrops-system-contracts.md Show resolved Hide resolved

doc: address pr comments

b5b615b

Signed-off-by: Stanimir Stoyanov <[email protected]>

stoyanov-st marked this pull request as ready for review September 13, 2024 07:01

stoyanov-st requested review from a team as code owners September 13, 2024 07:01

stoyanov-st requested review from jasperpotts, povolev15 and lukelee-sl September 13, 2024 07:01

Nana-EC reviewed Sep 17, 2024

View reviewed changes

docs: address pr comments

0cc5dd6

Signed-off-by: Stanimir Stoyanov <[email protected]>

stoyanov-st requested a review from Nana-EC September 17, 2024 12:08

david-bakin-sl previously approved these changes Sep 17, 2024

View reviewed changes

david-bakin-sl reviewed Sep 17, 2024

View reviewed changes

docs: update

5a58667

Signed-off-by: Stanimir Stoyanov <[email protected]>

stoyanov-st dismissed david-bakin-sl’s stale review via 5a58667 September 17, 2024 16:50

docs: add missing class def

28dbf02

Signed-off-by: Stanimir Stoyanov <[email protected]>

Nana-EC reviewed Sep 18, 2024

View reviewed changes

docs: improvements

8eb75b9

Signed-off-by: Stanimir Stoyanov <[email protected]>

stoyanov-st requested a review from Nana-EC September 24, 2024 07:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: Provide Design document for HIP-904 System Contracts flows #15435

docs: Provide Design document for HIP-904 System Contracts flows #15435

stoyanov-st commented Sep 11, 2024 •

edited

Loading

codacy-production bot commented Sep 11, 2024 •

edited

Loading

codecov bot commented Sep 11, 2024 •

edited

Loading

Nana-EC left a comment

Nana-EC Sep 17, 2024

stoyanov-st Sep 17, 2024

david-bakin-sl Sep 17, 2024

david-bakin-sl left a comment

david-bakin-sl Sep 17, 2024

david-bakin-sl Sep 17, 2024 •

edited

Loading

david-bakin-sl Sep 17, 2024

Nana-EC Sep 18, 2024

david-bakin-sl Sep 17, 2024 •

edited

Loading

david-bakin-sl Sep 17, 2024

lukelee-sl commented Sep 17, 2024

Nana-EC left a comment

Nana-EC Sep 18, 2024

		\| `0xc1acfe5d` \| `function cancelAirdrops(PendingAirdrop[] memory pendingAirdrops) external returns (int64 responseCode)` \| `ResponseCode` \| The response code from the call \|
		\| `0x241b6af7` \| `function claimAirdrops(PendingAirdrop[] memory pendingAirdrops) external returns (int64 responseCode)` \| `ResponseCode` \| The response code from the call \|

docs: Provide Design document for HIP-904 System Contracts flows #15435

Are you sure you want to change the base?

docs: Provide Design document for HIP-904 System Contracts flows #15435

Conversation

stoyanov-st commented Sep 11, 2024 • edited Loading

codacy-production bot commented Sep 11, 2024 • edited Loading

Coverage summary from Codacy

codecov bot commented Sep 11, 2024 • edited Loading

Codecov Report

Nana-EC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-bakin-sl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-bakin-sl Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-bakin-sl Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukelee-sl commented Sep 17, 2024

Nana-EC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stoyanov-st commented Sep 11, 2024 •

edited

Loading

codacy-production bot commented Sep 11, 2024 •

edited

Loading

codecov bot commented Sep 11, 2024 •

edited

Loading

david-bakin-sl Sep 17, 2024 •

edited

Loading

david-bakin-sl Sep 17, 2024 •

edited

Loading