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

Supports witnesses using domains for their nodes in addition to IPs #17

Merged
merged 11 commits into from
Jul 9, 2023

Conversation

Rishi556
Copy link
Contributor

@Rishi556 Rishi556 commented Feb 8, 2023

No description provided.

@Rishi556 Rishi556 changed the title Supports witnesses using domains for their nodes rather than IPs Supports witnesses using domains for their nodes in addition to IPs Feb 8, 2023
Copy link
Contributor

@bt-cryptomancer bt-cryptomancer left a comment

Choose a reason for hiding this comment

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

Being able to register by both domain and IP will be a nice bit of added flexibility. Please also add support for using domains in the witness_action.js utility script, and see additional comments below.

contracts/witnesses.js Outdated Show resolved Hide resolved
libs/util/testing/TableAsserts.js Outdated Show resolved Hide resolved
test/witnesses.js Show resolved Hide resolved
contracts/witnesses.js Show resolved Hide resolved
contracts/witnesses.js Outdated Show resolved Hide resolved
Copy link
Contributor

@bt-cryptomancer bt-cryptomancer left a comment

Choose a reason for hiding this comment

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

Thanks for addressing earlier feedback. Mostly looks good now, just a couple minor things remaining; see comments below.

witness_action.js Outdated Show resolved Hide resolved
witness_action.js Show resolved Hide resolved
Copy link
Contributor

@bt-cryptomancer bt-cryptomancer left a comment

Choose a reason for hiding this comment

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

Everything looks good now. Will merge for inclusion in the next release once you are satisfied with the testing.

@Rishi556
Copy link
Contributor Author

We ran this over the weekend, on a testnet(and plan to leave running until tomorrow at least) and saw it all go smoothly. Invalid cases(some inadvertently) were attempted as well, and all failed. I'm pretty comfortable releasing this, if you'd like to join in the testing, feel free to.

For release, we'll need a new tagged release first that witnesses upgrade to(due to changes in plugins/P2P), and then the contract should be deployed.

@bt-cryptomancer bt-cryptomancer merged commit b90f7a7 into hive-engine:qa Jul 9, 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
Development

Successfully merging this pull request may close these issues.

None yet

2 participants