-
Notifications
You must be signed in to change notification settings - Fork 162
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: Adding Meteor Wallet to the project #348
Conversation
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.
Hi @lostpebble thanks for submitting this PR.
Can you please change the base form main
to dev
branch?
Can you also update the main Readme file to include meteor-wallet
too?
For the near-api-js
errors regarding Class Action, have you tried to match the versions?
Like using the same version of near-api-js in your project to match with the version we have in this repo?
Thanks for the review @kujtimprenkuSQA ! I think I've covered all those bases- and changed to the |
Hey this looks good, it's just that when you changed the base from If you have any difficulties with this let me know. Also when the window opens to connect Meteor Wallet even though I am trying to connect from localhost it shows this site: |
Ah shucks, that's a debugging thing that I hard-coded and forgot to remove. I'll change that now to function as expected. Thanks for pointing it out!
Cool, will look into merging that now. |
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.
Few comments from me 👍
Disclaimer: This was purely a code review - I've not yet pulled the repo down and tested it so apologies if some of my comments are irrelevant!
I seem to be hitting the following error when building the repo:
Along with this error:
|
Hi @lewis-sqa , thanks for much for the review so far!
Yea, I noticed this error popping up now after rebasing our branch onto I'm in the process of trying to solve this, nothing has changed on our side- or perhaps while I was rebasing I missed something. |
I've found the "export" issue: vercel/next.js#31518 Turns out Next.js doesn't automatically convert ES Modules, like Webpack and other do. And when I change the
Now I get this error:
Seems the SDK is not getting pre-bundled with the other code, and the code is using |
I think I'll need to re-compile our SDK to commonjs for it to work. |
I changed our module and now I'm getting this error:
This is because of this code in our SDK: const locallySetBaseUrl = window.localStorage.getItem("DEV__METEOR_WALLET_BASE_URL");
export const envConfig: IEnvConfig = {
wallet_base_url: locallySetBaseUrl ?? WALLET_URL_PRODUCTION_BASE,
}; This code is always expected to execute inside the browser... but thanks to Next.js this is now throwing an error. I've run into things like this before with them- very annoying since clearly we're building for browser but window is not allowed because it executes within a Node.js context. |
Might be worth adding a condition around Web related APIs to make this module somewhat isomorphic. We actually moved to Next.js recently to ensure Wallet Selector handles this correctly as SSR seems to have caused problems for people in the past |
That's fair, better to error early. I've wrapped it with a check now. |
…an up module, name etc.
I've made a lot of edits now. It should be working, as the examples worked for me. Just need to look at some small final things recommended by @lewis-sqa about |
…ck for signerId while signing transactions to match current account
@lewis-sqa @kujtimprenkuSQA I think I've finished adding all the requested changes and verified that its working with the example. Please check and let me know if everything is up to scratch :) |
Shucks, always the small things that slip through! 😛 Cool, think I've changed the name in all the relevant places and removed that |
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.
This looks great. Thanks for all your efforts @lostpebble! 🎉
Thanks so much for the reviews and helping to get this merged @lewis-sqa and @kujtimprenkuSQA ! Happy to be a part of the ecosystem :) |
Is there any ETA on the next version release btw? So we can get our integrators to start testing it out |
@lostpebble Thanks the PR, we are planning Wallet-Selector 5.1.0 on Tuesday, August 16. We are planning to add this PR and enable Meteor Wallet. Cheers |
Hi @janewang - for a minor addition like this, this seems like a very long release schedule. I also see a minor version was released in-between without our new changes. We are trying to iterate fast, and for this we need our integrators to start using Wallet Selector so they can test out Meteor on their dapps. This PR was merged 19 days ago now- yet there is still seemingly 15 days to go until the decided release date (more than a month total to get a version with our wallet included)... Could this also have something to do with the big official change over to MyNearWallet (I read something about a big release announcement on the 17th of August)? This is a big blocker for us, as we've pushed for Wallet Selector to be our main integration with our users. For us to take our next beta steps we really need this functionality to be live now. Please could you consider another minor version that includes our current changes? |
Hey @lostpebble There is definitely significant resources devoted to various initiatives. We are looking to make sure the releases we announce can go out in time. Unfortunately the team working on wallet-selector is quite short handed esp given August vacations and natural attrition. Will discuss internally on what we can do. |
Thanks @janewang - appreciate the work of everyone involved! |
Please kindly confirm that the follow criteria are met or can be met within the required timeline. Thank you. Criteria for Including New Wallets for Wallet Selector Product Criteria:A wallet project must have comply the following product criteria to listed on Wallet Selector.
Security Criteria:A wallet project must have the majority of these security program features in place with self-certification that remaining items will be in place within 6 months of being listed. Wallets shall checkbox a statement of compliance to be maintained on the wallet’s github account. When asserting that open items will be in place within six months of being listed, target dates shall be included. The overall statement must be dates and the date shall be commensurate with the commit date. All wallet projects must maintain compliance. If a wallet is no longer in compliance, or no longer supported, the security statement must reflect that change. The wallet project shall make verification of the following requirements as easy as possible maximizing transparency through the use of relevant links pointing toward program descriptions, audit reports, etc. for the user/researcher.
|
Thanks for the criteria update @janewang. I've created a public repo (https://github.com/Meteor-Wallet/meteor-public) where we will put all the required documentation and things, and let you know once we're ready. We'll also be updating the wallet according to the "Injected Wallet Standard"- although I did have a few concerns about the API there which I have mentioned on the specific PR for that document ( near/NEPs#370 )- it seems that it is deviating from the API given in the current version of the Wallet Selector repo. We really need to get merged as soon as possible because not being on Wallet Selector is putting us in a tough spot with integrators- who we've been pushing "wallet selector" on as our main integration. I hope that we will be up to standard for the next release- we're a little disappointed that this criteria only landed right now, but I'm guessing this is a new process on your side that has only just been wrapped up. |
@janewang @lewis-sqa we will be implementing our new changes in accordance with the current Wallet Selector interfaces- using our internal SDK to communicate with our extension and website (depending on context, mobile, desktop etc.). As all wallets in the wallet selector currently implement those interfaces, we hope that will be fine to allow us to be integrated in the next version. |
@lostpebble Great to hear that you are planning to update to the wallet standards for product and security. We are planning to list meteor wallet in the upcoming release next week. |
Description
This adds Meteor Wallet to the Wallet Selector project. I've used the other modules as a template and added Meteor configuration in all the relevant places.
Checklist:
Type of change
Breaking changes
Documentation
I've you'd like more documentation on the Meteor SDK, you can find that here. It implements an interface very similar to Near Wallet, as seen inside
near-api-js
.Small Issues during integrationEDIT: Solved with next commit (adding
@meteorwallet/sdk
into the rootpackage.json
- following whatnightly-connect
has done)The wallet is live and ready for you guys to test this integration out :)