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

feat(api): Support sending assets from contract #158

Merged
merged 4 commits into from
Feb 4, 2018

Conversation

slipo
Copy link
Contributor

@slipo slipo commented Feb 1, 2018

Solves #147

This PR allows you to send assets from a contract. It supports Contract and Invocation transactions (which are used in neo-boa examples such as https://github.com/CityOfZion/neo-boa/blob/master/boa/tests/src/WithdrawTest.py#L100).

I saw very similar functionality for mintTokens. I'm unsure how mintTokens was being used in this case. If we need it to work as before, is there a way to test it? It will be easy to add back in the exact same way as before if needed.

For my implementation I set invocationScript to '00' * number of contract parameters which is what the mintTokens version was doing. I did not append the main script's invocationScript as the issue says as this does not seem to be needed. The original mintToken implementation was using the contract's script hash in the attribute and I needed to use the sender's. Again, it wasn't clear to me how mintToken was working. What we have here is much more like how neo-python works with --from-addr when sending.

Here is some testing code:
https://gist.github.com/slipo/c1542c087bff7e17a067e903f0b24540

Here's a resulting transaction:
https://gist.github.com/slipo/6b2815058652fce6b386267f1b880639

@snowypowers
Copy link
Member

yes we need the mintToken functionality. Please update it such that the old functions are retained and make your own new ones. They serve very different functions although they look similar. The mintToken function allows the contract to reject the transaction if the token sale is oversubscribed / finished. We are adding features to allow control over SC assets.

Do make a distinction for a smart contract. A normal send asset transaction is called ContractTransaction because an address is considered a normal contract.

src/api/core.js Outdated
/**
* Adds attributes to the override object for mintTokens invocations.
* @param {object} config - Configuration object.
* @return {object} Configuration object.
*/
const addAttributesForMintToken = config => {
Copy link
Member

Choose a reason for hiding this comment

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

We want our mintToken functionality.

@@ -157,6 +157,33 @@ describe('Core API', function () {
})
})

it('sign for contract assets', () => {
Copy link
Member

Choose a reason for hiding this comment

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

do use SC or smart contract instead of just contract. An normal address is a normal contract.

@slipo
Copy link
Contributor Author

slipo commented Feb 3, 2018

Thank you for the feedback. I've made all changes.

I see that mintTokens is a special case where the assets are coming from the sender's regular address but they want the verification triggered. I've put all original mintTokens code back.

I also have created a sendAssets example:
https://gist.github.com/slipo/b4ff4569ada4a93fce683c3da512e0f2

Please see my question in that code. It seems to me we need to (option 1, shown in example) pass along a scriptHash when doing this form of send assets. Otherwise we need to (option 2) remove this:

    if (config.address !== acct.address && !sendingAddressIsSmartContract(config)) {
      return Promise.reject(
        new Error('Private Key and Balance address does not match!')
      )
    }

Or a third option is to add a new config.sendingFromSmartContract = true. I would personally prefer options 1 or 3 because I think that this isn't the most common usage and that error message could be useful for people.

@snowypowers
Copy link
Member

yes, I think adding a sendingFromSmartContract field would be the option here. We do not want to deviate from the original purpose of sending assets from a personal account. Another solution could be to create a new method called sendAssetsFromSC in order to differentiate. I will accept either methods for now.

@slipo
Copy link
Contributor Author

slipo commented Feb 4, 2018

Done. Looking cleaner to me.

@snowypowers snowypowers merged commit 480312b into CityOfZion:dev Feb 4, 2018
@snowypowers
Copy link
Member

Thanks for the work! There might be some changes as to where this should live but that should be minor.

@slipo slipo deleted the send-from-contract branch February 4, 2018 03:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants