-
Notifications
You must be signed in to change notification settings - Fork 278
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
Add /wallet/:id/tx http call #465
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.
I think that this functionality is redundant and can be accomplished with endpoints that already exist.
-
POST /wallet/:id/send
Line 420 in 22be7fa
this.post('/wallet/:id/send', async (req, res) => { -
POST /wallet/:id/create
Line 433 in 22be7fa
this.post('/wallet/:id/create', async (req, res) => {
Note that POST body can include an outputs
array of objects that represent each output, and a covenant
JSON property can be included in the output object.
Lines 1642 to 1653 in 22be7fa
if (valid.has('outputs')) { | |
const outputs = valid.array('outputs'); | |
for (const output of outputs) { | |
const valid = new Validator(output); | |
let addr = valid.str('address'); | |
if (addr) | |
addr = Address.fromString(addr, this.network); | |
let covenant = valid.obj('covenant'); |
|
||
// @todo: Is this safe without a lock around it? What needs and does not need locks isn't | ||
// super clear. | ||
outputs.push(await req.wallet.makeBidOutput(name, bid, lockup, options.account || 0)); |
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.
Nit: separate this into multiple lines instead of using ||
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.
Agree, was just copying the style that's used in the codebase.
Things would be much easier with prettier/typescript, but that's a separate issue.
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.
eslint
is used for this project, you can see the configuration here:
https://github.com/handshake-org/hsd/blob/master/.eslintrc.json
@qix Are you able to accomplish what you would like with the endpoints mentioned in #465 (review)? |
Yeah sorry about the delay. Not sure how I missed that earlier, happy to close this one out. It's a little unfortunate that the wallet tx API requires a lot more handshake protocol specifics, but I can make it work. In any case this change would be much neater/shorter by adding a |
@tynes re-opening this because the user experience of the other API is far from ideal. I'm of the opinion that the ability to send multiple bids in a transaction is a useful API for a wallet. While I could go ahead and implement my own wallet (perhaps using hsd as a library), I'd rather build on top of the hsd API. Trying to send a transaction using To focus on the issue at hand after some time trying to get it to work this is the call I came up with:
There is a lot there that is API specific, and requires multiple calls to the RPC to fetch the address and the nameHash. Instead I propose an extension to
That frees the user from having to know protocol specifics (bid = type 3), generating the hashes or fetching them from the API, understanding the height parameter of the covenant (honesty haven't had a chance to read about what that should be set to), or similar concerns. I'm happy to implement it, believe that change would be shorter than the diff here. |
I think you're on the right track @qix and you're right the create API requires the user to retrieve data the wallet usually handles on its own. The one thing I don't like about your proposed example is the This reminds me of So, maybe what would suit your use case best is a very specific new rpc
an object where keys are names and values are an array of If the usecase calls for it, maybe similar rpc |
I think that this is an interesting idea, what do you think about generalizing it so that it can create a transaction with arbitrary opens, bids or reveals? Technically all of the covenant types could be included, but that increases the complexity. |
That's why I suggested adding it to a more generic endpoint. I imagined That way support for |
I'm somewhat against the |
maybe it can be complexified:
|
Works for me if that's the approach you all want. I prefer the HTTP api for extensibility reasons (adding another type of I do think the arguments to |
I think the most useful approach is the most general and implements all output types. The current endpoint allows to send arbitrary output types, but the user has to enter all covenant items, like sha3 of the name or its hex encoding. All this data can be added by the node. What do you think of I fell that more natural would be to use |
That looks good to me too, but to be honest I'm not planning on investing more time in this project so going to sit out of further discussion. |
I think this looks the best and is the most obvious. the arguments following each covenant type would match the So then in wallet/rpc.js we add that call, with a switch statement for each covenant type that checks with the rpc Then each of those from Lines 1699 to 1711 in a3049d5
|
At the moment it is not possible to create a single transaction with multiple bids (or multiple opens.) This creates a new RPC endpoint that allows the caller to pass in an array of bids. The bids are specified exactly the same as the
bid
endpoint requires them.This should be extendable to allow
opens
,reveals
, and all other covenant types to be specified in a later change.I have not heavily tested this change, but have verified that it works. If the approach is correct I can add some test cases.