Skip to content
This repository has been archived by the owner on Jan 30, 2024. It is now read-only.

rewrite #10

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

matthiasdebernardini
Copy link

Rewrote the whole thing, if this seems unsuitable I can keep it as is and put in changes to existing.

The reason for the rewrite was that I had too many weird errors while working with the site and I had to look up how to make a flask project.

Copy link
Contributor

@elsirion elsirion left a comment

Choose a reason for hiding this comment

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

Did you try to build using nix? Don't have time rn, but would be cool if you could take a look at elsirion/nix-bitcoin@6890ee5 and how it works.

return render_template('index.html', name='justin',
invoice=invoice, pay_result=pay_result, connect_str=connect_str)

from app import app
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to have something like the following here:

if __name__ == "__main__":
    app.run(port = 3000)

so that it can be started without some weird other component I never got to run on Nix (I know you aren't supposed to, but don't want to waste more time on this).

Choose a reason for hiding this comment

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

I was passing the port in as an option because the default port was conflicting with fedimint (5000)

Yes I can take a look at the nix and get it to work.

Copy link
Contributor

Choose a reason for hiding this comment

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

An option would be great! This is just what I did so far.

@matthiasdebernardini
Copy link
Author

Scanner needs fixing

import os

class Config(object):
SECRET_KEY = os.environ.get('SECRET_KEY') or 'you-will-never-guess'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is needed because you aren't using the session at all

Comment on lines +5 to +6
RPC_SOCKET = os.environ.get('RPC_SOCKET') or 'you-will-never-guess'
CONNECT_INFO = os.environ.get('CONNECT_INFO') or 'you-will-never-guess'
Copy link
Collaborator

Choose a reason for hiding this comment

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

The app should probably fail if these aren't set. It just won't work. So let's not set defaults here.

Copy link
Contributor

Choose a reason for hiding this comment

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

It does fail, the or syntax doesn't work. Defaults have to be given to get as a second argument.

@elsirion
Copy link
Contributor

I got it to work/deployed using 4c5701d.

Copy link
Contributor

@elsirion elsirion left a comment

Choose a reason for hiding this comment

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

See 4c5701d on how to make it work with nix. Needs some design fixes but that's less important. High prio:

  • Limit withdraw amount
  • return errors

<img style="height: 200px; width: 200px;" src="{{ qrcode(connect_str) }}">
<br>
<pre>{{ connect_str }}</pre>
<br>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add some description back in about what this is and how to use it.



@app.route('/')
@app.route('/index')
Copy link
Contributor

Choose a reason for hiding this comment

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

why make this also /index?

from json2html import *

rpc = LightningRpc(os.environ['RPC_SOCKET'])
connect_str = os.environ['CONNECT_INFO']
Copy link
Contributor

Choose a reason for hiding this comment

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

should come from config instead

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants