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

Implement @radspec helper #55

Merged
merged 10 commits into from Feb 22, 2019

Conversation

Projects
None yet
2 participants
@izqui
Copy link
Member

commented Feb 4, 2019

The @radspec helper allows to recursively execute radspec as part of a radspec string. These strings should be either included in the radspec package or loaded from a trusted registry, as the recursive nature of this feature could be used to crash radspec with an infinite loop.

@izqui izqui requested a review from sohkai Feb 4, 2019

@sohkai sohkai added this to the A1 Sprint: 4.1 milestone Feb 4, 2019

izqui and others added some commits Feb 4, 2019

this.ast = ast
this.bindings = bindings
this.eth = new Eth(ethNode || 'https://mainnet.infura.io')
this.eth = eth || new Eth(ethNode || 'https://mainnet.infura.io')

This comment has been minimized.

Copy link
@sohkai

sohkai Feb 8, 2019

Member

Hmm, are we ever passing eth around? It should just be ethNode.

This comment has been minimized.

Copy link
@izqui

izqui Feb 8, 2019

Author Member

Helpers have the eth object so they can do network requests, but they don't have the ethNode. I guess there is a method to get it, but this seemed cleaner to me.

if (!fn) {
return {
type: 'string',
value: `Unknown (${methodId})`

This comment has been minimized.

Copy link
@sohkai

sohkai Feb 8, 2019

Member

Maybe we could also add the address here as well?

This comment has been minimized.

Copy link
@izqui

izqui Feb 8, 2019

Author Member

Makes sense for errors, although radspec strings don't print anything about the target contract.

How I see this being used in the Actor's execute for example is: Execute '@radspec(target, data)' on 'target'

const functions = processFunctions(require('../data/knownFunctions'))

// Get method ID
const methodId = data.substr(0, 10)

This comment has been minimized.

Copy link
@sohkai

sohkai Feb 8, 2019

Member

We should probably do some error checking that the data is formatted correctly

*/
async (addr, data) => {
// lazily import radspec to avoid a dependency cycle
const { evaluateRaw } = require('../index')

This comment has been minimized.

Copy link
@sohkai

sohkai Feb 8, 2019

Member

.__. not sure how well this is going to fly with builders like parcel. Will need to check.

This comment has been minimized.

Copy link
@izqui

izqui Feb 8, 2019

Author Member

🤷‍♀️ cleaner way I could think of that didn't require a major re-architecture

{
"setOwner(address)": "Set `$1` as the new owner",
"setOwner(bytes32,address)": "Set `$2` as the new owner of the `$1` node",
"transfer(address,address,uint256)": "Transfer `@tokenAmount($1, $3)` to `$2`"

This comment has been minimized.

Copy link
@sohkai

sohkai Feb 8, 2019

Member

Not generally against this idea, but find it a bit unnecessary.

Instead of re-evaluating radspec, we could just run a javascript function here with the arguments to build the string. We could inject the web3 instance as well, if we really wanted to allow web3 calls.

@sohkai sohkai removed feedback labels Feb 11, 2019

@radspec: add fallback to Parity on-chain signature registry (#57)
* @radspec: add fallback to Parity on-chain signature registry

* Style comments

* Update methodRegistry.js

@izqui izqui changed the title [WIP] Implement @radspec helper Implement @radspec helper Feb 12, 2019

@izqui

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2019

All good from my side. @sohkai PTAL!

@izqui izqui referenced this pull request Feb 12, 2019

Merged

Agent app: EVM #580

5 of 5 tasks complete
@sohkai

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

@izqui Will test locally to see if the recursive require becomes a problem for frontends.

Otherwise, I think we should still fix #55 (comment); the relative brittleness of how we handle radspec errors means the more error checking we do, the better.

sohkai added some commits Feb 21, 2019

@sohkai sohkai merged commit f020a29 into master Feb 22, 2019

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
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.