bigchaindb / js-bigchaindb-driver

Official BigchainDB JavaScript driver for Node.js and the browser
https://docs.bigchaindb.com/projects/js-driver/en/latest/usage.html
Apache License 2.0
216 stars 92 forks source link

Delegated signature #304

Closed getlarge closed 3 years ago

getlarge commented 3 years ago

Implement logic from @zzappie in Python driver.

codecov-io commented 3 years ago

Codecov Report

Merging #304 (3447022) into master (ea57280) will increase coverage by 0.15%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #304      +/-   ##
==========================================
+ Coverage   95.16%   95.31%   +0.15%     
==========================================
  Files          13       13              
  Lines         310      320      +10     
==========================================
+ Hits          295      305      +10     
  Misses         15       15              
Impacted Files Coverage Δ
src/transaction.js 97.77% <100.00%> (+0.27%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update f08ea00...3447022. Read the comment docs.

davie0 commented 3 years ago

Thank you @getlarge! Looks great!

We need to come to conclusion on what to pass to the signing callback. I made lazy choice of passing the whole tx mapping. And you additionally pass input field. I'll make adjustments to python-driver pr and report back today.

Any thoughts on what the interface supposed to be?

getlarge commented 3 years ago

Since inside the delegateSignTransaction we iterate over the transaction inputs, i thought it make sense to pass the current input to eventually filter a set of keys by checking the owners_before.

I also think it make sense to pass the whole the transaction if someone needs information to perform the signature, it will be available in the callback.

I'd like to also make this function's interface working for partial signature of a transaction, but my brain is still stuck on understanding all the tenants.

getlarge commented 3 years ago

@zzappie So should we modify the delegateSignTransaction ?

davie0 commented 3 years ago

I'm doubtful that there is a use case for passing the whole transaction. It doesn't mean there is no such case.

But lets go through other tx fields. "id": non existent at point of delegation "version": Maybe we need it? "outputs": Non existent at point of delegation "operation": Doesn't matter "asset" and "metadata: We already got message prepared

I think that if someone really will need something more than input to know which private key to use for sighing. They could achieve it build key selection logic that happens prior to delegation. So in my opinion to keep clean interface we can go with just input and message.

WDYT?

getlarge commented 3 years ago

Your summary makes sense. Do you know if version has an influence on the type of signature to provide, or there were changes of the expected signatures/fulfillement across versions of BigChain ?

When you mean message, you talk about the transaction hash right ?

davie0 commented 3 years ago

Your summary makes sense. Do you know if version has an influence on the type of signature to provide, or there were changes of the expected signatures/fulfillement across versions of BigChain ? I think information present in input should be enough for now. 2.0 spec is going to be "the latest" for a while, and driver versioning is not closely tied to spec versioning. So we have the room to adapt if we find edge cases. And one of the design goals for this feature was to keep exposure of driver an bigchaindb tx spec specific things to minimum to the callback. I allready pushed updates to bigchaindb-driver.

When you mean message, you talk about the transaction hash right ? Yep right.

One thing though. According to our CLA you need to sign-off the commits You cand do so by running git rebase --signoff HEAD~6

And then force push the branch. It will add Signded-off by: Your Name your@e.mail to commit messages.

An we'll be ready to merge!