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

Wrapper: Add error log when radspec evaluation fails #392

Merged
merged 2 commits into from Oct 2, 2019

Conversation

@gh1dra
Copy link
Contributor

commented Sep 30, 2019

Fixes #101.

We could also take this opportunity to add tests to describeTransaction but I'm not sure where to put them so that they are aligned with the pattern. Also creating stubs for radspec could be additional work outside the scope of this small PR.

@welcome

This comment has been minimized.

Copy link

commented Sep 30, 2019

Thanks for opening this pull request! Someone will review it soon 🔍

@auto-assign auto-assign bot requested review from 2color and sohkai Sep 30, 2019
} else {
return {
...transaction
}

This comment has been minimized.

Copy link
@gh1dra

gh1dra Sep 30, 2019

Author Contributor

Was trying to encapsulate the logic here: #101 (comment)

Though this may not be needed, as if the evaluatedNotice is undefined, tryEvaluatingRadspec returns the fields inside the transaction as well:

let evaluatedNotice
if (method && method.notice) {
try {
evaluatedNotice = await radspec.evaluate(
method.notice,
{
abi,
transaction: intent
},
{ ethNode: wrapper.web3.currentProvider }
)
} catch (err) {
console.error(`Could not evaluate a description for given transaction data: ${intent.data}`, err)
}
}
return {
...intent,
description: evaluatedNotice
}

This comment has been minimized.

Copy link
@sohkai

sohkai Oct 2, 2019

Member

Yes, this part isn't needed because we're only interested in returning the description or annotatedDescription in this API and not the entire transaction (it's already given as input).

Although I did find a bug with how we're handling the description here, since tryEvaluatingRadspec returns an object with a description key and not just the string itself.

…andler
@sohkai sohkai changed the title Add error message logging to radspec evaluation resolving #101 Wrapper: Add error message when radspec evaluation fails Oct 2, 2019
@sohkai
sohkai approved these changes Oct 2, 2019
Copy link
Member

left a comment

Thanks @gh1dra!

We can leave the tests for another PR. Eventually we'll want to move to a better logging structure, see #197, but having this log is better than not for now :).

@sohkai sohkai changed the title Wrapper: Add error message when radspec evaluation fails Wrapper: Add error log when radspec evaluation fails Oct 2, 2019
@sohkai sohkai merged commit c167834 into aragon:master Oct 2, 2019
3 of 4 checks passed
3 of 4 checks passed
coverage/coveralls Coverage decreased (-0.06%) to 55.404%
Details
WIP Ready for review
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@welcome

This comment has been minimized.

Copy link

commented Oct 2, 2019

Congrats on merging your first pull request! Aragon is proud of you 🦅
Eagle gif

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.