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 node contract #106

Merged
merged 1 commit into from
Aug 13, 2024
Merged

Update node contract #106

merged 1 commit into from
Aug 13, 2024
+1,301 −188

Conversation

neekolas
Copy link
Contributor

@neekolas neekolas commented Aug 10, 2024 •
edited
Loading

TL;DR

  • Rewrites Nodes contract using the ERC721 standard as a base
  • Adds a bunch of tests for the new smart contract
  • Removes a deprecated flag from our mockery config

Why ERC721

  1. It doesn't seem crazy to model this registry as a NFT, so we might as well use the standard.
  2. We get all the OpenZeppelin audited smart contract libraries for free
  3. It'll work with the entire rest of the crypto ecosystem (external indexers, other smart contracts, etc) automatically

AI Assisted Changelog

  1. Smart Contract Changes:

    • Implemented ERC721 standard in the Nodes contract.
    • Added new functions for node management using NFTs: addNode, removeNode, updateHttpAddress, updateMtlsCert, and updateHealth.
    • Added events NodeUpdated and NodeRemoved for better tracking of node changes.
    • Restricted node ownership transfer.

  2. Go Bindings Update:

    • Updated the ABI bindings to reflect new changes in the Nodes contract.
    • Added data structures NodesNode and NodesNodeWithId to handle the new Node data.
    • Implemented necessary function bindings for node management in Go.

  3. Testing:

    • Added new test cases in Solidity to cover the new node management functionalities.
    • Renamed test contract from CounterTest to GroupMessagesTest and added NodesTest for comprehensive testing of node functionalities.

This was referenced Aug 10, 2024
Merged
Merged
@neekolas Graphite App
Copy link
Contributor Author

neekolas commented Aug 10, 2024 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @neekolas and the rest of your teammates on Graphite Graphite

@neekolas neekolas mentioned this pull request Aug 10, 2024
Merged
@neekolas neekolas marked this pull request as ready for review August 10, 2024 21:57
@neekolas neekolas force-pushed the 08-10-update_node_contract branch 3 times, most recently from 6044dc8 to 31b5549 Compare August 11, 2024 00:49
@neekolas neekolas mentioned this pull request Aug 11, 2024
Merged
@neekolas neekolas force-pushed the 08-10-scaffold_smart_contract_event_storage branch from 87eaedc to c4246b6 Compare August 12, 2024 21:38
@neekolas neekolas force-pushed the 08-10-update_node_contract branch 2 times, most recently from 5d7b02f to 0efeea1 Compare August 12, 2024 21:53
@neekolas neekolas mentioned this pull request Aug 12, 2024
Merged
@neekolas neekolas changed the base branch from 08-10-scaffold_smart_contract_event_storage to graphite-base/106 August 12, 2024 22:59
@neekolas neekolas changed the base branch from graphite-base/106 to main August 12, 2024 22:59
neekolas
neekolas commented Aug 12, 2024
View reviewed changes
contracts/test/Nodes.sol
node.httpAddress
);

vm.prank(operator);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

vm.prank is cool. It just lets you change the msg.sender value to any other address for a test, so you can impersonate someone and see how the contract behaves.

neekolas
neekolas commented Aug 12, 2024
View reviewed changes
pkg/registry/registry.go
NodeId int
SigningKey []byte
HttpAddress string
MtlsCert []byte
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This field goes away in the next PR

richardhuaaa
richardhuaaa approved these changes Aug 13, 2024
View reviewed changes
Copy link
Contributor

@richardhuaaa richardhuaaa left a comment

Choose a reason for hiding this comment

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

Using ERC721 is a smart idea. Will ecosystem tools break if we don't specify base/token URI? I suppose we could also generate some fun images too. Also do we need to block safeTransferFrom?

contracts/src/Nodes.sol
bytes calldata signingKeyPub,
string calldata httpAddress
) public onlyOwner returns (uint16) {
uint16 nodeId = _nodeIdCounter;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not allow a node to have node ID 0, because an originator SID of 0 means that the originator is the blockchain. Wondering if we should pre-increment _nodeIdCounter rather than post-incrementing it?

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 have a comment somewhere about this. I kinda think we should start at 1000 just to be safe. Would let us partition the smart contracts later if we wanted to.

contracts/src/Nodes.sol
Comment on lines +66 to +69
require(
_msgSender() == owner(),
"Only the contract owner can transfer Node ownership"
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Small nit, any reason this one uses a require statement, but updateHealth uses public onlyOwner?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only that I wanted a better error message, but agree the inconsistency feels weird

contracts/src/Nodes.sol
require(
bytes(nodes[publicKey].httpAddress).length == 0,
"Node already exists"
_msgSender() == ownerOf(tokenId),
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 there are tradeoffs to allowing/encouraging nodes to update the HTTP address at any point, rather than going through a more heavyweight process via the DAO? For example, ensuring there is a graceful failover etc. Don't have a super strong opinion here

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 don't really know. Wondering the same thing. Worth a discussion.

@neekolas neekolas merged commit 155819e into main Aug 13, 2024
9 checks passed
@neekolas neekolas deleted the 08-10-update_node_contract branch August 13, 2024 00:32
neekolas
neekolas commented Aug 13, 2024
View reviewed changes
Copy link
Contributor Author

@neekolas neekolas left a comment

Choose a reason for hiding this comment

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

Will ecosystem tools break if we don't specify base/token URI?

The OpenZeppelin default just sets it to "". I would expect tools to handle that, since some generative NFTs are on-chain only. But I'm with you that at some point we'll do an extra-credit project and make a URI that has JSON metadata with a fun image and more info. Then it fully looks like a normal NFT.

Also do we need to block safeTransferFrom?

safeTransferFrom calls transferFrom internally, so we get them both with this override.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardhuaaa richardhuaaa richardhuaaa 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.
2 participants
@neekolas @richardhuaaa