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.

Sign up for GitHub

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

Jump to bottom

Update HIP-904 status and HSCS details #1080

Merged
merged 4 commits into from
Nov 20, 2024
Merged

Update HIP-904 status and HSCS details #1080

merged 4 commits into from
Nov 20, 2024

Conversation

stoyanov-st
Copy link
Contributor

@stoyanov-st stoyanov-st commented Nov 14, 2024 •
edited
Loading

Description:

  • Update status to Final
  • Update HIP-904 with the Smart Contract function selectors and structures .

@stoyanov-st
Update HIP-904 HSCS details
c576fec 
Signed-off-by: Stanimir Stoyanov <[email protected]>
@stoyanov-st stoyanov-st requested a review from Nana-EC November 14, 2024 15:29
@stoyanov-st stoyanov-st self-assigned this Nov 14, 2024
@stoyanov-st stoyanov-st requested a review from mgarbs as a code owner November 14, 2024 15:29
@netlify Netlify
Copy link

netlify bot commented Nov 14, 2024 •
edited
Loading

✅ Deploy Preview for hedera-hips ready!

Name Link
🔨 Latest commit b5a7d24
🔍 Latest deploy log https://app.netlify.com/sites/hedera-hips/deploys/673725f57ca3580008ccfe04
😎 Deploy Preview https://deploy-preview-1080--hedera-hips.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@stoyanov-st
Update date
79a4b9c 
Signed-off-by: Stanimir Stoyanov <[email protected]>
@stoyanov-st
Update HIP status
c41474e 
Signed-off-by: Stanimir Stoyanov <[email protected]>
@stoyanov-st stoyanov-st requested a review from jnels124 November 14, 2024 16:43
@stoyanov-st stoyanov-st changed the title Update HIP-904 HSCS details Update HIP-904 status and HSCS details Nov 14, 2024
@steven-sheehy steven-sheehy mentioned this pull request Nov 14, 2024
Closed
steven-sheehy
steven-sheehy previously approved these changes Nov 14, 2024
View reviewed changes
jnels124
jnels124 reviewed Nov 14, 2024
View reviewed changes
Copy link
Contributor

@jnels124 jnels124 left a comment

Choose a reason for hiding this comment

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

LGTM

Nana-EC
Nana-EC reviewed Nov 14, 2024
View reviewed changes
Copy link
Contributor

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

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

2 suggestions for completion.
Thanks

HIP/hip-904.md Outdated
@@ -7,11 +7,12 @@ requested-by: DEXs, Wallets, External Web3 Users. Notably Reality+, Liithos, Dro
type: Standards Track
category: Core
needs-council-approval: Yes
status: Accepted
status: Final
release: v0.54.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be v0.56 as that's when the system contract functions went in.
Agreed it's 0.54 for when HAPI functionality was completed but it might be confusing if users test this using local node or solo 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense. I copied it from the previous PR.

HIP/hip-904.md Show resolved Hide resolved
lukelee-sl
lukelee-sl previously approved these changes Nov 14, 2024
View reviewed changes
Copy link
Member

@lukelee-sl lukelee-sl left a comment

Choose a reason for hiding this comment

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

LGTM

david-bakin-sl
david-bakin-sl reviewed Nov 14, 2024
View reviewed changes
HIP/hip-904.md
address receiver;

address token;
int64 serial;
Copy link
Member

@david-bakin-sl david-bakin-sl Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It seems totally obvious that serial is only for NFTs - but still, maybe a comment that addresses that (and states explicitly that that depends on the token type addressed by the token field)? (Reason is: It's a little odd that the very next lines introduce NftID for identifying "the Nft serial to be rejected" but there's nothing similar for fungible types (the address only?) and anyway it's not used here in PendingAirdrop because that's the way solidity works: No union types.). And must serial == 0 for fungible tokens or is it a don't care?

Another funny thing with NftID is why isn't it used as the argument for cancelAirdropNFT or claimAirdropNFT? But it isn't I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment for clarification.
For the latter comment, the stated functions are redirect calls that already have the token address so it makes it obsolete to use NftID there. The address parameters there are the token airdrop sender/receiver as stated in the naming.

Copy link
Member

Choose a reason for hiding this comment

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

ah! my mistake

@stoyanov-st
Update HIP
b5a7d24 
Signed-off-by: Stanimir Stoyanov <[email protected]>
@stoyanov-st stoyanov-st dismissed stale reviews from lukelee-sl and steven-sheehy via b5a7d24 November 15, 2024 10:44
david-bakin-sl
david-bakin-sl approved these changes Nov 15, 2024
View reviewed changes
Copy link
Member

@david-bakin-sl david-bakin-sl left a comment

Choose a reason for hiding this comment

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

lgtm

Nana-EC
Nana-EC approved these changes Nov 18, 2024
View reviewed changes
Copy link
Contributor

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

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

LG

@mgarbs mgarbs merged commit 2eacaf5 into hashgraph:main Nov 20, 2024
9 checks passed
netopyr added a commit that referenced this pull request Dec 4, 2024
@netopyr
Merge branch 'main' into hip-991-update
2ac2b5b 
* main:
  Daily rewards for active nodes (#1064)
  HIP-980: Add shadow fork support as a rejected idea (#1058)
  Update HIP-904 status and HSCS details (#1080)
@stoyanov-st stoyanov-st deleted the sstoyanov-update-hip-904 branch January 30, 2025 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@david-bakin-sl david-bakin-sl david-bakin-sl approved these changes

@mgarbs mgarbs mgarbs approved these changes

@Nana-EC Nana-EC Nana-EC approved these changes

@steven-sheehy steven-sheehy steven-sheehy approved these changes

@lukelee-sl lukelee-sl Awaiting requested review from lukelee-sl

@jnels124 jnels124 Awaiting requested review from jnels124

Assignees

@stoyanov-st stoyanov-st

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@stoyanov-st @jnels124 @steven-sheehy @Nana-EC @mgarbs @lukelee-sl @david-bakin-sl