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.

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

Pulsechain Network Addition #12948

Closed
wants to merge 10 commits into from
Closed

Pulsechain Network Addition #12948

wants to merge 10 commits into from

Conversation

JexxaJ
Copy link
Contributor

@JexxaJ JexxaJ commented Nov 6, 2023

Description

Issues

This PR address this issue #9304 (comment)

Checklist:

  • 1 PR, 1 purpose: my Pull Request applies to a single purpose
  • I have commented my code, particularly in hard-to-understand areas
  • I have updated the docs to reflect my changes if applicable
  • I have added tests (and stories for frontend components) that prove my fix is effective or that my feature works
  • I have performed a self-review of my own code
  • If my code involves visual changes, I am adding applicable screenshots to this thread

Copy link

cla-bot bot commented Nov 6, 2023

Thank you for your pull request and welcome to Unlock! We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @JexxaJ on file.
In order for us to review and merge your code, please open another pull request with a single modification: your github username added to the the file .clabot.
Thank you!

@clemsos clemsos requested review from julien51 and clemsos November 6, 2023 13:01
@julien51
Copy link
Member

julien51 commented Nov 6, 2023

Thanks @JexxaJ Please open another PR with the required changed to the CLA file!
We will review ASAP!

export * from './linea'
export * from './sepolia'
export * from './pulsechain'
export * from './pulsechainTestnetV4'
Copy link
Member

Choose a reason for hiding this comment

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

We try to not add all testnets because that means we have to maintain them.
Any reason why Goerli or Sepolia are not enough?

Copy link
Contributor Author

@JexxaJ JexxaJ Nov 6, 2023

Choose a reason for hiding this comment

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

All the development that is done on pulsechain is tried and tested using pulsechain testnet as it replicates the particular way in which the system state was forked over form Eth mainnet. If possible it would be great to see it added as it will foster development using the unlock protocol.

Copy link
Member

Choose a reason for hiding this comment

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

We won't add it as it means more work for our team... if Pulsechain is not compatible with Ethereum test nets then we should not add support for it.


export const pulsechain: NetworkConfig = {
chain: 'pulsechain',
description: 'pulsechain mainnet.',
Copy link
Member

Choose a reason for hiding this comment

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

We need a better description here so users know what is specific about Pulsechain please!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add something more detailed.

fullySubsidizedGas: false,
id: 369,
isTestNetwork: false,
keyManagerAddress: '',
Copy link
Member

Choose a reason for hiding this comment

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

TK

isTestNetwork: false,
keyManagerAddress: '',
maxFreeClaimCost: 10000,
multisig: '',
Copy link
Member

Choose a reason for hiding this comment

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

TK

IT looks like Gnosis Safe does not support this chain... can you please confirm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct they have not included pulsechain on the UI, however this network is one of the biggest users of gnosis safes via
https://staker.app/ which is the UI used on pulsechain and ethereum.

The liquid loans team have deployed a cut down version of gnosis on both testnet and mainnet at the below URLs.
https://pulsechainsafe.com/
https://testnet.pulsechainsafe.com/

Let me know if you need more information on these.

Copy link
Member

Choose a reason for hiding this comment

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

At this point we don't want to add more work to our team to maintain non standards setup.

symbol: 'PLS',
},
previousDeploys: [],
provider: 'https://rpc.unlock-protocol.com/369',
Copy link
Member

Choose a reason for hiding this comment

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

We would need to deploy a provider for this.

previousDeploys: [],
provider: 'https://rpc.unlock-protocol.com/369',
publicLockVersionToDeploy: 13,
publicProvider: 'https://scan.pulsechain.com',
Copy link
Member

Choose a reason for hiding this comment

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

This is a block explorer. We need an RPC endpoint here.

provider: 'https://rpc.unlock-protocol.com/369',
publicLockVersionToDeploy: 13,
publicProvider: 'https://scan.pulsechain.com',
startBlock: 2247300,
Copy link
Member

Choose a reason for hiding this comment

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

TK

publicLockVersionToDeploy: 13,
publicProvider: 'https://scan.pulsechain.com',
startBlock: 2247300,
// Graph can be found at https://graph.pulsechain.com/
Copy link
Member

Choose a reason for hiding this comment

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

Since this is not a standard https://thegraph.com/ setup you need to send us docs on how to set it up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will try and get some more info for you on this side of things. Is this a nice to have or required?

Copy link
Member

Choose a reason for hiding this comment

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

Required for the frontend to work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so there is a community graph node that can be used. I have spoken to the team running this and they have advised while there is no end-user UI at present if you are able to provide the subgraph code you want hosted they will run this up.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry this is too complex for us to maintain so until there is a proper graph deployment we won't merge this PR.

networkName: 'pulsechain',
studioEndpoint: 'unlock-protocol-pulsechain',
},
unlockAddress: '',
Copy link
Member

Choose a reason for hiding this comment

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

TK

Copy link
Member

@julien51 julien51 left a comment

Choose a reason for hiding this comment

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

This is incomplete. Please remove the testnet as well.

Copy link

cla-bot bot commented Nov 6, 2023

Thank you for your pull request and welcome to Unlock! We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @JexxaJ on file.
In order for us to review and merge your code, please open another pull request with a single modification: your github username added to the the file .clabot.
Thank you!

Copy link

cla-bot bot commented Nov 6, 2023

Thank you for your pull request and welcome to Unlock! We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @JexxaJ on file.
In order for us to review and merge your code, please open another pull request with a single modification: your github username added to the the file .clabot.
Thank you!

Copy link

cla-bot bot commented Nov 6, 2023

Thank you for your pull request and welcome to Unlock! We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @JexxaJ on file.
In order for us to review and merge your code, please open another pull request with a single modification: your github username added to the the file .clabot.
Thank you!

Copy link

cla-bot bot commented Nov 7, 2023

Thank you for your pull request and welcome to Unlock! We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @JexxaJ on file.
In order for us to review and merge your code, please open another pull request with a single modification: your github username added to the the file .clabot.
Thank you!

Copy link

cla-bot bot commented Nov 7, 2023

Thank you for your pull request and welcome to Unlock! We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @JexxaJ on file.
In order for us to review and merge your code, please open another pull request with a single modification: your github username added to the the file .clabot.
Thank you!

@cla-bot cla-bot bot added the cla-signed label Nov 10, 2023
@JexxaJ
Copy link
Contributor Author

JexxaJ commented Nov 13, 2023

This is incomplete. Please remove the testnet as well.

Can you please have another look over this request and let me know if anything is still outstanding? thanks.

@julien51
Copy link
Member

Closing as stale

@julien51 julien51 closed this Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants