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

Add CAIP-2 for Solana #60

Merged
merged 5 commits into from
Aug 16, 2021
Merged

Add CAIP-2 for Solana #60

merged 5 commits into from
Aug 16, 2021

Conversation

romeo4934
Copy link
Contributor

@romeo4934 romeo4934 commented Aug 4, 2021 •
edited
Loading

CAIP-2 for Solana !

@romeo4934
Copy link
Contributor Author

Hello! Any feedbacks regarding CAIP-2 for Solana 👀

@romeo4934
Copy link
Contributor Author

@pedrouid @ligi ? :)

ligi
ligi requested changes Aug 10, 2021
View reviewed changes
CAIPs/caip-30.md Outdated
caip: 30
title: Blockchain Reference for the Solana Namespace
author: Antoine Herzog (@antoineherzog), Josh Hundley (@oJshua)
discussions-to: []
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
discussions-to: []
discussions-to: https://github.com/ChainAgnostic/CAIPs/pull/60

CAIPs/caip-30.md Outdated

## Simple Summary

This document is about the details of the Solana namespaces and references for CAIP-2.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This document is about the details of the Solana namespaces and references for CAIP-2.
This document is about the details of the Solana namespace and reference for CAIP-2.

@ligi
Copy link
Member

ligi commented Aug 10, 2021

sorry was on the road for a bit - just some minor change suggestions.
And I found it a bit weird that there is a getGenesisHash function. Sounds a bit weird to have a specail function for this and not use the standard get block by number for block 0.

@romeo4934
Copy link
Contributor Author

No prob! I've just addressed for your feedbacks.
Solana also have the get block by number function however Joshua told me that we could use the getGenesisHashfunction.

@romeo4934
Copy link
Contributor Author

@pedrouid any feedback?

@pedrouid
Copy link
Member

LGTM 👍 Thanks for submitting it @antoineherzog. I was also going to need this 😅

@romeo4934
Copy link
Contributor Author

@ligi lets merge?

@ligi ligi merged commit 9b72330 into ChainAgnostic:master Aug 16, 2021
@ligi
Copy link
Member

ligi commented Aug 16, 2021

@pedrouid better leave a approval than a LGTM comment - makes it easier to see if there are 2 approvals than reading the comments.

@pedrouid
Copy link
Member

@pedrouid better leave a approval than a LGTM comment - makes it easier to see if there are 2 approvals than reading the comments.

My bad. You’re right

@clehner clehner mentioned this pull request Sep 9, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ligi ligi ligi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@romeo4934 @ligi @pedrouid