Suggestion regarding siwe-parser usage of apg-js.

ldthomas commented 1 year ago

Hi. I just have a couple of suggestions about the siwe-parser usage of apg-js. I know nothing about Ethereum or Sign in with Ethereum and you may (even probably) have already considered these and have good reasons for doing things the way you already do. But these may make your program a little more efficient and faster and maybe even reduce some dependencies.

1.) Why do you generate the grammar object each time you parse a msg? Why not generate the grammar object once and then simply import it each time you need it? Here is how that might look. Put the ABNF grammar in a separate file, say, ./dist/siwe-abnf.txt. Then generate the grammar object with

../../apg-js/bin/apg.sh -i ./dist/siwe-abnf.txt -o ./lib/siwe-grammar.js

Now in ./lib/abnf.ts replace your parsing line with the equivalent of

const obj = require('./siwe-grammar');
const grammarObj = new obj();
// const result = parser.parse(GrammarApi.grammarObj, "sign-in-with-ethereum", msg);
const result = parser.parse(grammarObj, "sign-in-with-ethereum", msg);

I don't know if speed and performance are an issue with you, but unnecessarily generating a grammar object each time processes the grammar through five phases, three of which are non-trivial.

2.) I think I know the answer to this but I will mention it anyway. I haven't figured out exactly where or how you use uri-js and valid-url but I notice that when you parse the message, you simply pull out the entire URI string and presumably parse it and verify it later with the dependency packages. Your grammar includes RFC 3986 so why not simply add a few more callback functions to the AST and pull out scheme, userinfo, host, port, path, query and fragment in the above parser.parse() statement? Why parse it twice? I'm guessing the answer is security and the special features you get with those other packages but wanted to point out the double parsing and ask anyway.

w4ll3 commented 1 year ago

Hi @ldthomas, sry for the delay in answering this. 1) TBH didn't know it was possible to have that seems it might improve performance, and yes it is important for us.

2) So for uri-js and valid-url we needed validation for some fields and those libraries did solve the problem. I'm considering moving that validation to apg so that we can remove some dependencies.

ldthomas commented 1 year ago

1) The stand-alone generator of a fixed grammar object was the only option until a couple of years ago. I got some complaints that the I/O (fs) was causing some people problems so I split it into an API to eliminate that for them. But if you have a stable ABNF grammar, using a pre-generated, fixed grammar object is definitely the way to go.

2) I'm not a URL expert but if apg-js parses your msg without failure then you know it contains a valid URI. It shouldn't be too hard then to do a few other examinations of the scheme, etc. that you get from the AST to see that they satisfy your other semantic requirements. https not ftp or some such thing.

ldthomas commented 12 months ago

Hi @w4ll3,

I've taken the liberty of rewriting the siwe parser. As I said, I know nothing about Etherium but I have had a lot of experience writing APG parsers. If you are interested we can figure out a way to get it incorporated into your repository. I could probably reconfigure it as a pull request. At the moment you can find it at siwe-suggestion. Its main features are:

It runs about twice as fast as your original parser.
It eliminates the valid-url and uri-js dependencies.
It fixes some deficiencies. The current parser fails on some URIs, particularly those with IPv6addresses.
It also addresses some silent errors with the IPv4address that can happen but go unnoticed because of a peculiarity in the way the host is defined.

I've added a large set of unit tests. I've included all of the RFC 3986 unit tests from uri-js (t-uri-js.test.ts). This was very informative and is where I picked up most of the problems with IPv6address. For example, try parsing an siwe message with

"URI: uri://[2001:db8::7]\n".

Your parser will throw an exception. The unit tests in t-ipv4-ipv6.test.js give IPv6address parsing a good work out. And as a side note, valid-url thinks this URI is just fine:

"uri://[ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff:255.255.255.255]".

There are some other subtleties with the way host is defined and the way IPv4address behaves there, but I won't go into those here. I've added:

import * as fs from "node:fs"; import { cwd } from "node:process";

to abnf.ts for debugging output. If you chose to trace the parser you will need those to see the trace. You may want to eliminate those from a production parser.

Let me know if you have any interest in this and we can discuss how best to incorporate it into your repository.

Best regards.

gilbert commented 6 months ago

Hey @ldthomas, would you put your work on a PR? I'm interested in the improvements you have made.

ldthomas commented 6 months ago

@Gilbert. Interesting. I haven't seen any activity on this repository at all since my last post 6 mo ago. I was beginning to think it was an abandoned project. I have a small project I need to finish up and then I'll need to get myself back up to speed on what I've done here. But yes, I should be able to come up with a PR in the next week or two if that works for you.

ldthomas commented 6 months ago

@gilbert. BTW. Where do you fit into this project? I don't see your name anywhere as a contributor or fork owner. Do you have write access to merge PRs?

sbihel commented 6 months ago

Apologies for the lack of communication. @gilbert isn't associated with SpruceID or is a maintainer on this repo but they are right that a PR would be very welcomed.

ldthomas commented 6 months ago

Hi @sbihel. I see that you are the author of many commits. Thanks for that clarification. As I said, it may take a week or two but I will put together a PR as soon as I can.

ldthomas commented 5 months ago

Hi @sbihel. Before I create a fork and PR I'm doing a dry run on a simple clone of the latest version of spruceid/siwe. All is going well except that I am having a problem that possibly you can help me with.

In a nutshell, if I clone a fresh copy of spruceid/siwe in a clean directory, I'm not able to build siwe/lib/client.ts. Specifically,

mkdir test cd test git clone https://github.com/spruceid/siwe.git cd siwe npm install cd packages/siwe npm run build

results in the error: lib/client.ts:6:8 - error TS2307: Cannot find module '@spruceid/siwe-parser' or its corresponding type declarations.

For some reason this problem does not show up in my repository siwe-suggestion. Any suggestions?

spruceid / siwe

Suggestion regarding siwe-parser usage of apg-js. #177