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

docs: ETH Account Funding Guide #57

Merged
merged 5 commits into from
May 29, 2024
Merged

Conversation

sgerodes
Copy link
Contributor

@sgerodes sgerodes commented May 25, 2024

I have encountered several problem when doing the first setup. I have found the documentation not good enough to get started. Fixed the documentation. This FIx will help newcommers to seamlesly start start working with this project.

Copy link
Collaborator

@bh2smith bh2smith left a comment

Choose a reason for hiding this comment

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

Thanks @sgerodes for your suggestions. I've made a few inline comments and suggestions that could improve your contribution.

To summarize, I would propose:

  1. Remove the .env file creation description since it is already above in the Local Testing section.
  2. Remove the opensea example from the Local Testing section, since the user hasn't yet been instructed to fund their ETH account. We can then use your send-eth in the place you've included it a simpler script.
  3. Elaborate on the "Fund your account guide" by adding a getEthAddress script to the examples and instructing the user to run that in order to determine the ETH address associated with their near account.

Furthermore, this might be a good place to include a try-catch block directly in the project with a more human readable error that the account they are trying to build a transaction for is not funded:

This part here is the core of the error (when trying to get gas estimates for the transaction payload)

https://github.com/Mintbase/near-ca/blob/a1d3c557e19b68e13587a0d520b82813179fa1e3/src/utils/transaction.ts#L44-L49

However, it might not hurt to put a more generic try-catch around the call to populateTx here

https://github.com/Mintbase/near-ca/blob/a1d3c557e19b68e13587a0d520b82813179fa1e3/src/chains/ethereum.ts#L160-L164

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@sgerodes
Copy link
Contributor Author

@bh2smith I have changed the docs according to your review

Copy link
Collaborator

@bh2smith bh2smith left a comment

Choose a reason for hiding this comment

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

Hey this looks much better thanks. There is another PR that will conflict with a few of these changes, but otherwise it looks good. I guess you're not going to introduce the try-catch?

@bh2smith bh2smith changed the title docs: fix instructions for .env setup and running send-eth docs: ETH Account Funding Guide May 29, 2024
@bh2smith bh2smith merged commit 6232c8f into BitteProtocol:main May 29, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants