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

feat: Homogenize Bitcoin and Ethereum Examples #45

Merged
merged 3 commits into from
Dec 2, 2024
Merged

feat: Homogenize Bitcoin and Ethereum Examples #45

merged 3 commits into from
Dec 2, 2024

Conversation

gagdiez
Copy link
Collaborator

@gagdiez gagdiez commented Dec 2, 2024

With the last PR merged, our Bitcoin and Ethereum files diverged a lot, this PR makes them have the same interface, while addressing the issues raised in #40

The new structure further helps to easily replace all the current services for the multichain library - once it's finished

Another important fix is the use of a different RPC gateway for Ethereum, the current was returning the wrong amount of transactions per account - which made our nonce calculation wrong - as well as giving errors when broadcasting the transactions

@gagdiez gagdiez requested a review from PiVortex December 2, 2024 11:33
src/services/ethereum.js Outdated Show resolved Hide resolved
src/services/bitcoin.js Outdated Show resolved Hide resolved
Copy link
Contributor

@PiVortex PiVortex left a comment

Choose a reason for hiding this comment

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

I could not get the Bitcoin version to work because:

  1. It does not work for web wallet
  2. For meteor wallet there is an RPC timeout as it waits for the result https://gist.github.com/jaswinder6991/ec6a4bbae47418ea85d7c408df1357f8

Other comments include:

  • We should save in session storage what action (function call) is being used so the correct page is retained after wallet redirect
  • For ethereum and bitcoin we console log different things, probably best to log the same info
  • I would prefer there not to be the callMPC method in the wallet file, we tend to have the same wallet file in each example and if your coming from our other examples it might be confusing where this function is coming from and what exactly is happening.
  • I think for the deposit we should have it higher than 1 yocto near, on testnet it is most often higher than this. I think 0.1 NEAR would be appropriate.

@gagdiez gagdiez requested a review from PiVortex December 2, 2024 14:11
@gagdiez
Copy link
Collaborator Author

gagdiez commented Dec 2, 2024

@PiVortex thanks!

  • I fixed the imports.
  • I removed the callMPC as a method from the wallet, I agree that it is weird having it there
  • console logs are irrelevant
  • I moved the deposit one level higher, so we can set it up from the frontend in a future PR, the current

Bitcoin does not work for web wallet
For meteor wallet there is an RPC timeout
We should save in session storage what action is being used

Notice thought that those problems were already present in the code base and they were not being addressed by this PR, which simply tries to bring both ethereum.js and bitcoin.js interfaces closer!

But please, do open issues for those ones

@gagdiez gagdiez merged commit ca34d12 into main Dec 2, 2024
@gagdiez gagdiez deleted the homogenize branch December 2, 2024 14:33
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