Multiple assets support. - [merged] #374

Closed kwannoel closed 3 years ago

In GitLab by @isd on Aug 25, 2021, 06:50

Merges multi-assets -> master

Closes #7.

Corresponding gerbil-ethereum pr: https://github.com/fare/gerbil-ethereum/pull/51

...which will have to be merged before this will work.

In GitLab by @isd on Aug 26, 2021, 06:06

added 1 commit

8a7b9cd6 - Comment out accidentally committed debugging feature.

Compare with previous version

In GitLab by @plotnick on Aug 27, 2021, 05:15

added 1 commit

08ee3f9b - Use explicit "-p" option to provide port to nc

Compare with previous version

In GitLab by @plotnick on Aug 31, 2021, 04:13

added 1 commit

34a0f9ec - Don't use explicit "-p" option to provide port to nc

Compare with previous version

In GitLab by @isd on Sep 2, 2021, 05:17

added 1 commit

46bb40f1 - Transmit all assets on selfdestruct.

Compare with previous version

In GitLab by @fahree on Sep 6, 2021, 20:53

Commented on runtime/consensus-code-generator.ss line 489

Add TODO: to move that to gerbil-utils or gerbil (if not already available, e.g. in a SRFI).

In GitLab by @fahree on Sep 6, 2021, 20:53

Commented on runtime/participant-runtime.ss line 396

Does that mean that there is a potential race condition with other clients that would control the same key, and that in case of a race, the contract address will be wrong and the assets lost? That's definitely worth a bit fat WARNING in the comments at the very least and a TODO to fix the issue!

One strategy might be to make things atomic by depositing funds in a persistent user-controlled proxy contract rather than a contract with a transient address. Then, in an atomic transaction, fund the transient contract then create it.

In GitLab by @fahree on Sep 6, 2021, 20:53

Commented on t/asset-swap-integrationtest.ss line 57

I remember there were nc issues. Is this the definite solution?

Maybe we should have a builtin nc equivalent in glow, that would be portable?

Not for this PR. Is there a place for a TODO, though? Maybe near where the handshake option is defined?

In GitLab by @fahree on Sep 6, 2021, 20:53

Commented on t/asset-swap-integrationtest.ss line 62

TODO: there also ought to be a better automation than this answer-questions thing.

In GitLab by @fahree on Sep 6, 2021, 20:54

LGTM in general, modulo need for TODO comments.

In GitLab by @plotnick on Sep 6, 2021, 22:59

Commented on t/asset-swap-integrationtest.ss line 62

Yes, I had to go around it for the UI work; we should be able to make it run with an appropriate combination of env. variables and command-line flags. But let's take it as a follow-up.

In GitLab by @isd on Sep 7, 2021, 03:50

Commented on t/asset-swap-integrationtest.ss line 57

I'm going to open an issue instead; we have this in several places.

Edit: opened #215

In GitLab by @isd on Sep 7, 2021, 03:55

added 1 commit

b01fd8e8 - Add some TODO comments requested by Fare.

Compare with previous version

In GitLab by @isd on Sep 7, 2021, 03:55

Commented on runtime/participant-runtime.ss line 396

I think it's not quite that bad: this only approves the transfers but doesn't perform them, so if the other contract is also owned by us the tokens are probably recoverable. But this indeed is something that merits a TODO comment. I've added one, with a solution I think is a bit better.

In GitLab by @isd on Sep 7, 2021, 03:59

Commented on t/asset-swap-integrationtest.ss line 62

Note that originally we didn't invoke the CLI in these integration tests at all, instead calling the functions directly in-process. I think that should still be our preferred approach for invoking glow from other scheme code. We switched to doing things this way specifically to test the CLI, not because feeding answers to the CLI prompt is a good way to automate invoking glow.

In GitLab by @isd on Sep 7, 2021, 03:59

I think I've addressed and/or replied to everything.

kwannoel / urban-octo-carnival

Multiple assets support. - [merged] #374