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

staking: allocate use channel public key #190

Merged
merged 3 commits into from
May 13, 2020

Conversation

abarmat
Copy link
Contributor

@abarmat abarmat commented May 8, 2020

  • When allocating stake to subgraph the index node now must send the public key used in the payment channel.
  • Allocate() advertises the public key in the AllocationCreated event so we can capture in our Network Subgraph, using that information consumers can route payments to the index node.
  • Allocate() calculates the signer address of the index node in the payment channel by getting the Ethereum address from the public key.
  • The signer address is used, as before, as one of the parties in the channel multisig and is validated to avoid reuse.

NOTE:

Reported issues to both projects:

@abarmat abarmat requested review from davekaj and Jannis May 8, 2020 14:55
@abarmat abarmat self-assigned this May 8, 2020
require(isChannel(_channelID) == false, "Allocation: channel ID already in use");
// Cannot reuse a channelID that has been used in the past
/* prettier-ignore */
address channelID = publicKeyToAddress(bytes(_channelPubKey[1:])); // solium-disable-line
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you manage to verify that this is correct?

Two examples:
Public key 0x03a74c72b51f4d36e9e4a13616dddeae31bafe41b1662804476a6859e26d9c734f
Results in address: 0xcFFAc3084674cc596F1F91c16f8FBc2De16B3DC8

Public key 0x036b7b0e7896e59a8b9e0a940e25d253b18a25fefba08da3eeb19a25839ba65eea
Results in address: 0x72b7e00027212AEEe61B88d6aE714f278220b595

Copy link
Contributor

Choose a reason for hiding this comment

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

I've verified that we can go from the public key to the Connext channel identifier, so I can confirm that channelID / signer address and public key are all I need.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I verified it is working on the tests in test/staking/general.test.js

I used:

  • Public Key '0x0456708870bfd5d8fc956fe33285dcf59b075cd7a25a21ee00834e480d3754bcda180e670145a290bb4bebca8e105ea7776a7b39e16c4df7d4d1083260c6f05d53'
  • Address: '0x6367E9dD7641e0fF221740b57B8C730031d72530'

@Jannis note that the public key is in uncompressed format for Solidity to easily convert to address

Snippet on how to do that:

const publicKey = '0x026655feed4d214c261e0a6b554395596f1f1476a77d999560e5a8df9b8a1a3515';
let compressedPublicKey = ethers.utils.computePublicKey(publicKey, true);
let uncompressedPublicKey = ethers.utils.computePublicKey(publicKey, false);

console.log(compressedPublicKey);
// "0x026655feed4d214c261e0a6b554395596f1f1476a77d999560e5a8df9b8a1a3515"

console.log(uncompressedPublicKey);
// "0x046655feed4d214c261e0a6b554395596f1f1476a77d999560e5a8df9b8a1a35" +
//   "15217e88dd05e938efdd71b2cce322bf01da96cd42087b236e8f5043157a9c068e"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Jannis let me know if you are ok with the response so we can merge

@@ -159,6 +159,9 @@ contract('Staking', ([me, other, governor, indexNode, channelOwner]) => {
describe('staking', function() {
beforeEach(async function() {
this.subgraphId = helpers.randomSubgraphIdHex0x()
this.channelPubKey =
'0x0456708870bfd5d8fc956fe33285dcf59b075cd7a25a21ee00834e480d3754bcda180e670145a290bb4bebca8e105ea7776a7b39e16c4df7d4d1083260c6f05d53'
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this key here is an extended public key? It's much longer than a regular public key.

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've just see this comment and posted an explanation in the comment before.

It's called uncompressed public key format. I posted a snippet using the ethers library on how to convert from one public key format to the other.

@abarmat
Copy link
Contributor Author

abarmat commented May 8, 2020

I committed just one more change to bump to the newest version of solidity-prettier. They fixed the issue I reported.

Copy link
Contributor

@davekaj davekaj left a comment

Choose a reason for hiding this comment

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

LGTM, i made one small note on a comment

require(
stake.tokensAvailable() >= _tokens,
"Allocation: not enough tokens available to allocate"
);
// Only can allocate tokens to a subgraph if not currently allocated
Copy link
Contributor

Choose a reason for hiding this comment

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

Only can => Can only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed and rebased!

@abarmat abarmat force-pushed the feat/staking-allocate-public-key branch from c8c9d20 to e41de1e Compare May 8, 2020 21:40
abarmat added 3 commits May 13, 2020 14:40
…culate Ethereum address of the channel signer using the public key
@abarmat abarmat force-pushed the feat/staking-allocate-public-key branch from e41de1e to c4d3e4e Compare May 13, 2020 17:49
@abarmat abarmat merged commit 52df003 into master May 13, 2020
@abarmat abarmat deleted the feat/staking-allocate-public-key branch May 13, 2020 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants