Action.send(); #357

marcelomorgado · 2019-04-26T18:39:51Z

Issue link

#162

Auto-close the issue?

Closes #162

Types of changes

New feature (non-breaking change that adds functionality)

Breaking change (fix or feature that would cause existing functionality not to work as expected)

Notes

The send() function is async and the use of await on the call is necessary for now (Since we are still using waitForOneConfirmation()), it should be changed on the Test suite should capture how we expect the library to be used #219
The way that GnosisSafe was coded introduced an extra complexity to the Action object: A function that should be sync but creates an action from a sequence of async calls (See: GnosisSafe.executeTransaction()). The solution that I've found is assuming that the rawTx param (from Action constructor) could be a promise or not.
Maybe a better name could be used instead of rawTx (maybe rawAction?).

pcowgill · 2019-04-26T19:57:17Z

The send() function is async and the use of await on the call is necessary for now (Since we are still using waitForOneConfirmation()), it should be changed on the #219

Makes sense

The way that GnosisSafe was coded introduced an extra complexity to the Action object: A function that should be sync but creates an action from a sequence of async calls (See: GnosisSafe.executeTransaction()). The solution that I've found is assuming that the rawTx param (from Action constructor) could be a promise or not.

Makes sense! Thanks for this info, that's helpful.

Maybe a better name could be used instead of rawTx (maybe rawAction?).

Sounds good

packages/tasit-action/src/contract/Action.js

pcowgill · 2019-04-26T20:04:15Z

packages/tasit-action/src/contract/Action.js

+  }
+
+  _toRaw = async () => {
+    return await this.#rawTx;


Can we handle whether to await it or not upstream of this function somehow? Like only call _toRaw once we know the promise has resolved? Or only use this in tasit-contract-based-account?

I need to think about it, not sure.

For now, we are using that function only on tasit-contract-based-account GnosisSafe class.

Okay. Keep me posted. My intuition tells me that move would make sense, but I haven't thought it through fully.

Yep, I thought so. That's part of my rough reasoning for this.

I've made a change on this PR #360
On these changes, we are assuming that the Action will expect a sync rawTx on the constructor but the signAndSend function stills using await to resolve that. It isn't 100% solution, though.
Another path is to force rawTx to be sync (removing the await inside of the signAndSend but that'll make GnosisSafe.executeTransaction to be async (on the way that is written for now).

I don't have time to think this through fully right now - feel free to go with whatever you thinks makes the most sense for now, and I'll circle back to this soon

Or wait, maybe what I just said is wrong

I guess what I'm asking is this: At a high level, what is it about GnosisSafe.executeTransaction that prevents it from having a rawTx ready to go right away?

I guess what I'm asking is this: At a high level, what is it about GnosisSafe.executeTransaction that prevents it from having a rawTx ready to go right away?

The [#prepareTransaction](https://github.com/tasitlabs/TasitSDK/blob/4111170669afb4447cb95315f45fd4ecd1fa02ec/packages/tasit-contract-based-account/src/GnosisSafe.js#L157).
That function has some async calls needed to create a safe transaction (e.g.: GnosisSafeContract.nonce (L199) and GnosisSafeContract.getTransactionHash (L201)).
These async calls are nested and needed to create the transaction that we want to be executed by the GnosisSafeContract.
My intention is to have the GnosisSafe encapsulating all that is possible, transparent to the user.

transparent to the user or opaque to the user?

🤦‍♂️ I don't know why I've used the "transparent" word. Opaque was what I've meant. Thanks

packages/tasit-action/src/contract/Action.js

packages/tasit-action/src/contract/Contract.js

packages/tasit-action/src/erc721/ERC721Full.test.js

packages/tasit-contract-based-account/src/GnosisSafe.js

packages/tasit-contracts/src/scripts/populateDecentralandContracts.js

pcowgill · 2019-04-26T20:17:15Z

@marcelomorgado What does TBD: mean in the name of this PR?

marcelomorgado · 2019-04-26T22:21:25Z

@marcelomorgado What does TBD: mean in the name of this PR?

To Be Discussed

pcowgill · 2019-04-26T23:03:34Z

@marcelomorgado What does TBD: mean in the name of this PR?

To Be Discussed

Ah ok cool. It's often used to mean To Be Determined, so I was confused

packages/tasit-action/src/contract/Action.js

pcowgill · 2019-04-26T23:37:49Z

@marcelomorgado I don't think any of the open comment threads gate merging this. Nicely done with this PR, by the way! Any objection to merging it now?

marcelomorgado · 2019-04-27T00:07:29Z

@marcelomorgado I don't think any of the open comment threads gate merging this. Nicely done with this PR, by the way! Any objection to merging it now?

Let's merge!

marcelomorgado · 2019-04-30T17:01:35Z

packages/tasit-action/src/contract/Action.js

  #tx;
  #txConfirmations;
  #timeout;
  #lastConfirmationTime;

-  constructor(txPromise, provider) {
+  constructor(rawTx, provider, signer) {


Creating a thread for that:

Maybe a better name could be used instead of rawTx (maybe rawAction?).

Sounds good

To track easily.

Sounds good, thanks

marcelomorgado added 10 commits April 26, 2019 14:22

Styling

fefcce3

WIP

bcae487

Action.send()

bbaab95

Adjusting tasit-action testcases

cbd1e1c

WIP

907723f

Adjusting tasit-contract-based-account testcases

d99336f

Adjusting tasit-sdk testcases

1d6dd5a

Function renaming

e5631b5

Merge branch 'develop' into feature/action.send

6a7b81e

Skip non-deterministic

ea53439

pcowgill previously approved these changes Apr 26, 2019

View reviewed changes

marcelomorgado changed the title ~~TBD: Action.send();~~ Action.send(); Apr 26, 2019

promise to async/await

617a3d0

marcelomorgado dismissed pcowgill’s stale review via 617a3d0 April 26, 2019 22:42

marcelomorgado added 2 commits April 26, 2019 23:48

to -> address

4d47203

Review request

8cdb55b

pcowgill reviewed Apr 26, 2019

View reviewed changes

packages/tasit-action/src/contract/Action.js Show resolved Hide resolved

pcowgill approved these changes Apr 26, 2019

View reviewed changes

pcowgill merged commit 7d83e73 into develop Apr 27, 2019

pcowgill deleted the feature/action.send branch April 27, 2019 00:57

marcelomorgado mentioned this pull request Apr 30, 2019

Open threads #360

Merged

marcelomorgado commented Apr 30, 2019

View reviewed changes

marcelomorgado mentioned this pull request May 2, 2019

Rename Contract.wallet to Contract.account #374

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Action.send(); #357

Action.send(); #357

marcelomorgado commented Apr 26, 2019

pcowgill commented Apr 26, 2019

pcowgill Apr 26, 2019

marcelomorgado Apr 26, 2019

pcowgill Apr 26, 2019

marcelomorgado Apr 30, 2019

pcowgill Apr 30, 2019

pcowgill May 2, 2019

pcowgill May 2, 2019

marcelomorgado May 2, 2019

pcowgill May 2, 2019

marcelomorgado May 2, 2019 •

edited

Loading

pcowgill commented Apr 26, 2019

marcelomorgado commented Apr 26, 2019

pcowgill commented Apr 26, 2019

pcowgill commented Apr 26, 2019

marcelomorgado commented Apr 27, 2019

marcelomorgado Apr 30, 2019

marcelomorgado Apr 30, 2019

pcowgill Apr 30, 2019

Action.send(); #357

Action.send(); #357

Conversation

marcelomorgado commented Apr 26, 2019

Issue link

Auto-close the issue?

Types of changes

Notes

pcowgill commented Apr 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelomorgado May 2, 2019 • edited Loading

Choose a reason for hiding this comment

pcowgill commented Apr 26, 2019

marcelomorgado commented Apr 26, 2019

pcowgill commented Apr 26, 2019

pcowgill commented Apr 26, 2019

marcelomorgado commented Apr 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelomorgado May 2, 2019 •

edited

Loading