-
Notifications
You must be signed in to change notification settings - Fork 9
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
Document Ether.js for asset hub smart contracts #295
base: staging-ah-smart-contracts
Are you sure you want to change the base?
Document Ether.js for asset hub smart contracts #295
Conversation
|
||
Ensure you replace the `INSERT_MNEMONIC`, `INSERT_CONTRACT_ADDRESS` and `INSERT_ADDRESS_TO_CHECK` placeholders with actual values. Also, the contract ABI file (`Storage.json`) should be correctly referenced. | ||
|
||
## Conclusion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can add a follow up to the REMIX IDE as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, we can add REMIX IDE as a follow-up, but I see Remix more as a development environment. Ideally, we should link to some ethers.js tutorial that demonstrates how to implement a project using ethers.js. What do you think @CrackTheCode016 ?
In any case, those two options would be tasks to add later on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code snippets in this chapter are good examples to already be included in our strategy to be test-able right of the bat. We don't need to do it now, but cc-ing @UtkarshBhardwaj007 for discussion.
Note that my push to make things test-able will also force us all to use these tools e2e, which is good.
Similar to the rust setup, we can bundle such codes into a single NPM project for each chapter. npm run run
or similar should run a test script that would execute the flow e2e, and check the output.
Some notes about how to achieve this:
- We would need to rely on the console logs, capture them, and assert on them. For example, in this example, we should expect "Retrieved stored number: 42" to be in the logs
- We would need one seed account's private key to be stored in our gh as a secret with a lot of WND. This account can be used both for our manual testing, and in a CI to run this flow
- We should destroy the contract post test every time
Having written the above, it seems to me that this is non-trivial but achievable, so I won't necessarily suggest it now, but we should start thinking about it.
Agreed. I don't want to block this PR but a suggestion would be to write code that builds. Since we are creating tutorials, we could follow them to write code that actually builds and later on, as part of this issue we can add tests to ensure that the code remains valid as testing (like you pointed out @kianenigma ) might be non-trivial. I am working on the |
Co-authored-by: bader y <ibnbassem@gmail.com>
Co-authored-by: bader y <ibnbassem@gmail.com>
.snippets/code/develop/smart-contracts/evm-toolkit/ethers-js/checkStorage.js
Outdated
Show resolved
Hide resolved
Co-authored-by: 0xLucca <95830307+0xLucca@users.noreply.github.com>
Co-authored-by: 0xLucca <95830307+0xLucca@users.noreply.github.com>
Co-authored-by: 0xLucca <95830307+0xLucca@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a little love to give a more polished feel. Thank you!
Co-authored-by: Dawn Kelly <83190195+dawnkelly09@users.noreply.github.com>
@dawnkelly09 I've addressed your comments and I also aligned this branch with the (temporal) architecture present in https://github.com/polkadot-developers/polkadot-docs/tree/staging-ah-smart-contracts. This should be ready for final review now |
When I spun this up locally, the ethers.js page only renders the title and the feedback stuff. It's weird. |
Should be fixed in 103fa77 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few formatting nits plus one broken link. Thanks!
TODO: complete this index page |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sooo....we fixing this index page or nah? Obviously I don't want to merge it like this ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmm idk, to be honest. Since we are merging this into a branch that has a temporary Information Architecture (see this comment #300 (comment)), many things will probably change, particularly regarding indexes and so on.
I was considering making a separate PR when we have the final IA to update not only the website structure but also the index pages. WDYT, @dawnkelly09?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, if this is going to staging first that's probably ok. We just have to make sure there is a final check on TODOs before we move out of staging and deploy.
Co-authored-by: Dawn Kelly <83190195+dawnkelly09@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved for staging but needs work on index page(s) before it can be deployed publicly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing to consider is putting the variables at the top of the files, so users when they first look at the code snippets, they instantly see that they need to update those
Co-authored-by: Erin Shaben <eshaben@icloud.com>
This PR aims to add a basic documentation of ether.js to showcase the interaction with asset hub smart contracts
Note: The IA used is preliminary. We can circle back on this later on and define where all this content should be located