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

ENS documentation #587

Merged
merged 16 commits into from
Feb 4, 2022
Merged

ENS documentation #587

merged 16 commits into from
Feb 4, 2022

Conversation

SionoiS
Copy link
Contributor

@SionoiS SionoiS commented Jan 17, 2022

Hi,

I'm trying to deploy ENS contracts as an example but I'm having errors.

.expect("receipt can't be null after wait for confirmations; qed");

I'm not sure why the receipt it empty.

I will adding tests for most ENS functionality too.

@SionoiS
Copy link
Contributor Author

SionoiS commented Jan 18, 2022

I added tests with wasm-bindgen-test and by using a browser. It works great and the tests are actually useful.

I removed the example as I can't find a way to make it work.

edit: I'm not sure how to disable the automated testing for tests/web.rs

@tomusdrw
Copy link
Owner

@SionoiS please see #581, would that be helpful for you?

src/contract/ens/eth_ens.rs Outdated Show resolved Hide resolved
tests/web.rs Outdated
@@ -0,0 +1,161 @@
use wasm_bindgen::*;
Copy link
Owner

Choose a reason for hiding this comment

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

Why do these test need to be run in wasm_bindgen environment? Can't we use a mock transport?

Copy link
Contributor Author

@SionoiS SionoiS Jan 18, 2022

Choose a reason for hiding this comment

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

AFAIK with mock transport we can't know if the interface works with the contract in the wild. Testing against the real contract allow us to know if it changed.

The down side is that it can't be automated.

The best would be to deploy the contract to ganache then do the tests. I'll see what I can do tomorrow.

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 tried my best but maybe I should just scrap the test suite for ENS. I can't deploy the contracts, I don't understand what testing with mock transport would do since writing the response and the request will always succeed and I don't know how to automate the browser testing.

@SionoiS SionoiS marked this pull request as ready for review January 19, 2022 15:05
@SionoiS SionoiS changed the title ENS tests & example ENS documentation Jan 19, 2022
Copy link
Owner

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

Thanks a lot, looks pretty good!

src/contract/ens/mod.rs Outdated Show resolved Hide resolved
src/contract/ens/mod.rs Outdated Show resolved Hide resolved
src/contract/ens/public_resolver.rs Outdated Show resolved Hide resolved
@@ -19,6 +19,8 @@ pub enum Error {
/// An error during deployment.
#[display(fmt = "Deployment error: {}", _0)]
Deployment(crate::contract::deploy::Error),
/// Contract does not support this interface.
InterfaceUnsupported,
Copy link
Owner

Choose a reason for hiding this comment

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

Seems unused?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When check_interface_support return false its emitted.

Copy link
Owner

Choose a reason for hiding this comment

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

Can you point me to a line when it's emitted? I can't see this in that PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tomusdrw tomusdrw merged commit b6b9ebf into tomusdrw:master Feb 4, 2022
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.

2 participants