Skip to content
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

Omni transaction api #1289

Merged
merged 34 commits into from Jun 8, 2016

Conversation

Projects
None yet
3 participants
@Nevtep
Copy link
Contributor

Nevtep commented May 15, 2016

This is an API to provide native omnicore tx generation, class B is still being constructed inside omni. Class C has the payload generation done, but still need to create the tx from it.

Work in progress

if to_address==None or to_address==from_address:
#change goes to sender/receiver
print "Single extra fee calculation"
fee_total = Decimal(miner_fee) + Decimal(0.00005757*total_packets+0.00005757*total_outs) + Decimal(0.00005757) #exodus output is last

This comment has been minimized.

@dexX7

dexX7 May 15, 2016

Member

If possible, please don't hardcode any output values or dust thresholds.

This comment has been minimized.

@Nevtep

Nevtep May 16, 2016

Author Contributor

Good point, do you mean extract them into a constant? or is there a better way to get those values from omnicore?

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented May 15, 2016

FWIW, since you're using the omni_createpayload_xxx RPCs from Omni Core: there are also RPCs for the raw transaction creation, either class B or C: https://github.com/OmniLayer/omnicore/blob/omnicore-0.0.10/src/omnicore/doc/rpc-api.md#omni_createrawtx_opreturn (and following)

@Nevtep

This comment has been minimized.

Copy link
Contributor Author

Nevtep commented May 18, 2016

@dexX7 @achamely @zathras-crypto I've changed the API to rely on omnicore for createrawtx

I will test it this afternoon, if you see anything wrong please let me know

@Nevtep Nevtep force-pushed the Nevtep:omniTransaction-api branch from e1f6fa0 to 099313a May 19, 2016

@Nevtep Nevtep force-pushed the Nevtep:omniTransaction-api branch from 62124d2 to 370717e May 19, 2016

# get payload
payload = self.__generate_payload()
# Add exodous output
rawtx = createrawtx_reference(self.exodus_address)['result']

This comment has been minimized.

@dexX7

dexX7 May 19, 2016

Member

This is not needed. In case of OP_RETURN, we no longer need the Exodus address, and when adding multisig payloads with omni_createrawtx_multisig, then the Exodus output is added automatically.


#source script is needed to sign on the client credit grazcoin
hash160=bc_address_to_hash_160(self.rawdata['transaction_from']).encode('hex_codec')
prevout_script='OP_DUP OP_HASH160 ' + hash160 + ' OP_EQUALVERIFY OP_CHECKSIG'

This comment has been minimized.

@dexX7

dexX7 May 19, 2016

Member

I'm not sure what this is referring to, but it would be better, if the script is extracted (e.g. by iterating over the input transactions, if that makes sense in this context). We have to deal with pay-to-pubkey-hash, but also with pay-to-script-hash.

This comment has been minimized.

@Nevtep

Nevtep May 20, 2016

Author Contributor

@dexX7
This is what we do on the frontend with that script (sourceScript), I think it is needed like that for tx signing, but that's code we've been carrying around since the begining I think. do you think it's is not needed anymore?

            var bytes = Bitcoin.Util.hexToBytes(unsignedTransactionHex);
            var transaction = Bitcoin.Transaction.deserialize(bytes);
            var script = self.parseScript(sourceScript);

            if (transaction.ins.length == 0) {
                return {
                    waiting: false,
                    transactionError: true,
                    error: 'Error: Not enough inputs in the address!'
                };
            }
            transaction.ins.forEach(function(input) {
                input.script = script;
            });
            return transaction;

This comment has been minimized.

@dexX7

dexX7 May 21, 2016

Member

I'm not sure what's going to happen with the transaction etc., but if it's needed, then let's not remove it for now. :)

Also sorry, regarding my initial concern: since we only deal with pay-to-pubkey-hash addresses (those starting with 1) with Omni Wallet, I think we won't face the issue where we'd need to handle pay-to-script-hash addresses (starting with 3). We have to deal with both on a protocol level though, but this seems currently out of scope of OW.

rawtx = createrawtx_input(input['txid'],input['vout'],rawtx)['result']

# Add the change if above dust
if change > 5757:

This comment has been minimized.

@dexX7

dexX7 May 19, 2016

Member

The check change > 5757 can safely be removed, because omni_createrawtx_change doesn't add change, if it's considered as dust (and the original transaction is simply returned in this case, without error).

@Nevtep

This comment has been minimized.

Copy link
Contributor Author

Nevtep commented May 21, 2016

Ok Made some updates, Bitland crowdsale went live using this branch, multisig, I tryed simple send and got a successfull class C TX

The only thing I didn't changed from dexx comments was the prev_out script stuff, Added a comment there on the outdated diff, but code is still the same.

Let me know if we can keep it as is for now (since it works and it looks like it is needed on the client for signing) or if you want to dig through that and find out what happens if we take that from the inputs, but since this value is being used to replace the original input scripts, I think it is better to leave it there if no objections

@achamely anything left before we merge?

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented May 21, 2016

The prev_out script stuff shouldn't be a blocker. The parts I've seen look fine to me, although my review was limited.

@Nevtep Nevtep force-pushed the Nevtep:omniTransaction-api branch from 1fe2efb to fd30613 May 24, 2016

@achamely achamely added the Enhancement label Jun 8, 2016

@achamely

This comment has been minimized.

Copy link
Contributor

achamely commented Jun 8, 2016

looks good

@achamely achamely merged commit 20cf4e6 into OmniLayer:master Jun 8, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.