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

Integration of GSN with Sapphire #1

Merged
merged 11 commits into from
Jun 6, 2023
Merged

Integration of GSN with Sapphire #1

merged 11 commits into from
Jun 6, 2023

Conversation

xmzheng
Copy link

@xmzheng xmzheng commented Feb 21, 2023

the init changes for making GSN work on Sapphire

@xmzheng xmzheng requested a review from nhynes February 21, 2023 15:44
Copy link

@nhynes nhynes left a comment

Choose a reason for hiding this comment

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

This approach looks good. Could you please demonstrate it working end-to-end before we merge?

@@ -39,6 +40,7 @@ contract RelayHub is IRelayHub, Ownable, ERC165 {
using Address for address;

address private constant DRY_RUN_ADDRESS = 0x0000000000000000000000000000000000000000;
address private constant ZERO_ADDRESS = 0x0000000000000000000000000000000000000000;
Copy link

Choose a reason for hiding this comment

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

you can use address(0) for this

@@ -100,6 +105,15 @@ contract RelayHub is IRelayHub, Ownable, ERC165 {
batchGateway = _batchGateway;
relayRegistrar = _relayRegistrar;
setConfiguration(_config);
bytes memory randomBytes = Sapphire.randomBytes(32, "");
(bytes memory rawPublicKey, bytes memory rawPrivateKey) = Sapphire.generateKeyPair(Sapphire.SigningAlg.Ed25519Oasis, randomBytes);
Copy link

Choose a reason for hiding this comment

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

In the next runtime upgrade, you'll be able to use Sapphire.generateCurve25519KeyPair(), which is the correct thing to use.

relayData : relayRequest.relayData
});

if (relayRequest.request.from == ZERO_ADDRESS && relayRequest.request.to == ZERO_ADDRESS) {
Copy link

Choose a reason for hiding this comment

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

Suggested change
if (relayRequest.request.from == ZERO_ADDRESS && relayRequest.request.to == ZERO_ADDRESS) {
if (relayRequest.request.from == address(0) && relayRequest.request.to == address(0)) {

Comment on lines +111 to +112
publicKey!: string
cipher!: deoxysii.AEAD
Copy link

Choose a reason for hiding this comment

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

you will be able to use sapphire.cipher.X25519Deoxysii once the runtime upgrade happens and the key generation algs are aligned.

Comment on lines +167 to +170
const keypair = nacl.box.keyPair()
this.publicKey = hexlify(keypair.publicKey)
const sharedKey = await this.dependencies.contractInteractor.getHubSymmetricKey(this.publicKey)
this.cipher = new deoxysii.AEAD(arrayify(sharedKey))
Copy link

Choose a reason for hiding this comment

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

Please remember that this will be replaced with X25519DeoxysII from the compat lib!

@xmzheng xmzheng changed the title Xmzheng/sapphire Integration of GSN with Sapphire Mar 14, 2023
Copy link

@nhynes nhynes left a comment

Choose a reason for hiding this comment

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

If this does what it says it does on the tin and the questions in the comments are answered, it LGTM.

@@ -131,6 +133,10 @@ export class CommandsLogic {
timeout: 120000
})
provider.sendAsync = provider.send.bind(provider)
if (privateKey == null) {
Copy link

Choose a reason for hiding this comment

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

Suggested change
if (privateKey == null) {
if (privateKey === null) {

@@ -131,6 +133,10 @@ export class CommandsLogic {
timeout: 120000
})
provider.sendAsync = provider.send.bind(provider)
if (privateKey == null) {
privateKey = process.env.PRIVATE_KEY
console.log(`the process.env.PRIVATE_KEY is ${privateKey}`)
Copy link

Choose a reason for hiding this comment

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

Suggested change
console.log(`the process.env.PRIVATE_KEY is ${privateKey}`)

@@ -51,6 +51,8 @@ async function getProvider (
// provider-controlled private key
from = commander.from
console.log('using', from)
privateKey = '0x' + commander.privateKeyHex
console.log('privateKye is ', privateKey)
Copy link

Choose a reason for hiding this comment

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

probably do not want the changes in this file

@@ -9,6 +9,7 @@ gsnCommander(['n'])
(async () => {
try {
const network: string = commander.network
console.log(`network is ${network}`)
Copy link

Choose a reason for hiding this comment

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

(for the rest of the PR, please remove debugging console.logs)

@@ -286,9 +316,15 @@ contract RelayHub is IRelayHub, Ownable, ERC165 {
{
RelayCallData memory vars;
vars.initialGasLeft = aggregateGasleft();

// RelayHubValidator.verifyTransactionPacking(domainSeparatorName,rawRelayRequest,signature,approvalData);
Copy link

Choose a reason for hiding this comment

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

Can you remove this comment since it's been moved?

const depositValue = parseEther(env.deploymentConfiguration.paymasterDeposit)

if (paymasterBalance.toString() === '0') {
// if (paymasterBalance.toString() === '0') {
Copy link

Choose a reason for hiding this comment

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

is this not needed?

await deployer.deploy(StakeManager, 30000, 0, 0, '0x0000000000000000000000000000000000000001', '0x0000000000000000000000000000000000000001')
await deployer.deploy(Penalizer, 0, 0)
await deployer.deploy(StakeManager, 30000, 0, 0, '0x0000000000000000000000000000000000000001', '0x0000000000000000000000000000000000000001', {overwrite: false})
await deployer.deploy(Penalizer, 0, 0, {overwrite: false})
Copy link

Choose a reason for hiding this comment

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

hh-deploy should detect that these can be reused automatically?

@@ -37,6 +37,7 @@ const TestPaymasterConfigurableMisbehavior = artifacts.require('TestPaymasterCon

contract('RelayServer', function (accounts: Truffle.Accounts) {
const registrationRateSeconds = 500

Copy link

Choose a reason for hiding this comment

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

Suggested change

this.testToken = await TestToken.new()
this.stakeManager = await StakeManager.new(defaultEnvironment.maxUnstakeDelay, 0, 0, constants.BURN_ADDRESS, constants.BURN_ADDRESS)
this.penalizer = await Penalizer.new(defaultEnvironment.penalizerConfiguration.penalizeBlockDelay, defaultEnvironment.penalizerConfiguration.penalizeBlockExpiration)
console.debug("I am init here!")
Copy link

Choose a reason for hiding this comment

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

note

@@ -15,12 +15,24 @@ if (fs.existsSync(secretMnemonicFile)) {
secretMnemonic = fs.readFileSync(secretMnemonicFile, { encoding: 'utf8' })
}

const privateKey = '47e179ec197488593b187f80a00eb0da91f1b9d0b13f8733639f19c30a34926a'
Copy link

Choose a reason for hiding this comment

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

I hope that this isn't a private key you care about

@xmzheng xmzheng merged commit 9d221f5 into master Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants