-
Notifications
You must be signed in to change notification settings - Fork 126
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
docs: Provide Design document for HIP-904 System Contracts flows #15435
base: develop
Are you sure you want to change the base?
Conversation
Signed-off-by: Stanimir Stoyanov <[email protected]>
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesCodacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more |
Codecov ReportAll modified and coverable lines are covered by tests ✅
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 |
...a-node/docs/design/services/smart-contract-service/frictionless-airdrops-system-contracts.md
Show resolved
Hide resolved
...a-node/docs/design/services/smart-contract-service/frictionless-airdrops-system-contracts.md
Show resolved
Hide resolved
...a-node/docs/design/services/smart-contract-service/frictionless-airdrops-system-contracts.md
Show resolved
Hide resolved
Signed-off-by: Stanimir Stoyanov <[email protected]>
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.
1st pass thoughts
...a-node/docs/design/services/smart-contract-service/frictionless-airdrops-system-contracts.md
Outdated
Show resolved
Hide resolved
| `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 | |
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.
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
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.
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?
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.
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]>
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.
Some suggestions.
address receiver; | ||
|
||
address token; | ||
bool isNft; |
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.
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
.
| `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 | |
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.
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.)
} | ||
``` | ||
|
||
`NftId` - A struct that represents the Nft serials to be rejected. |
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.
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.
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.
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
- 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. |
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.
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.
| `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 | |
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.
This is HAS not HTS right? Does that need to be called out?
Signed-off-by: Stanimir Stoyanov <[email protected]>
Signed-off-by: Stanimir Stoyanov <[email protected]>
I pretty much agree with the comments by @Nana-EC and @david-bakin-sl. Nothing to add from me. |
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.
Good improvements.
3 remaining suggestions
} | ||
``` | ||
|
||
`NftId` - A struct that represents the Nft serials to be rejected. |
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.
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
Outdated
Show resolved
Hide resolved
...a-node/docs/design/services/smart-contract-service/frictionless-airdrops-system-contracts.md
Show resolved
Hide resolved
...a-node/docs/design/services/smart-contract-service/frictionless-airdrops-system-contracts.md
Outdated
Show resolved
Hide resolved
Signed-off-by: Stanimir Stoyanov <[email protected]>
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