statechannels / safe-protocol Goto Github PK

Not a part of this PR, but we should check that the caller sent the appropriate funds to escrow.

Originally posted by @kerzhner in #30 (comment)

I estimate probably 0.5 to 1 dev day to get our own gas accounting set up, following what we did here. I think it would be well worth the effort.

We could reuse quite a bit of the code that we already wrote.

Originally posted by @geoknee in #11 (comment)

Outside the scope of this PR but is lpAddress still hardcoded? Could we change that so the contract is initialized with an lpAddress?

Originally posted by @lalexgap in #73 (comment)

We've been moving fast so broad swathes of our prototype code is untested. Once we have the time we should spend some time writing a bunch of tests.

When a solidity contract function is invoked, ethers first attempts to estimate the gas needed for the transaction. If the function errors, a generic Error: cannot estimate gas; transaction may fail or may require manual gas limit error is thrown.

One workaround is to specify the gasLimit override for every function call. That's pretty cumbersome and easy to forget.

Is there a better way to avoid the generic errors and see useful revert reasons?

Create a practice proof of the HTLC vanilla swap protocol.

If Alice is unresponsive then Bob has to wait for the ticket to expire before being able to process the next transfer.

This is a pretty severe DoS vulnerability for Bob.

Colin started exploring a solution here

Get the SAFE white paper published to a preprint archive.

I think so? When an erc20 token is moved from L1 to L2, some "migration" contract either locks up or burns L1 erc20 tokens. Multiple of those "migration" contracts can exist for a token type.

It is likely not critical to think about this for the prototype. But I do want to flag this limitation.

Originally posted by @kerzhner in #98 (comment)

I still need to clean up the scripts and get everything checked in.

Originally posted by @lalexgap in #108 (comment)

Add a basic set of tests that ensure that the protocol works as expected.

We should add a basic unit test for the L2 functionality.

Currently SAFE V2 does not provide a way for Bob to skip swaps he doesn't want to commit to.

It might make the protocol easier to use and rule out some [edge cases] (https://www.notion.so/statechannels/HTLC-free-SAFE-e999aafe604c4ce3bc69aec1000971b0#d77f5774dd6b4aeaa05c824854ec48c2) if Bob can skip swaps.

If not possible, add a note to the README.

Older yarn versions format yarn.lock differently than newer yarn versions.

Currently we have a test for the happy path.

We should add a test for known unhappy paths as well.

We have a nice, consistent pattern across our other repos of setting up prettier.js to format according to some rules. (The rules can almost be arbitrary -- it doesn't matter).

Not having prettier auto-format on save means more cognitive overhead when developing, and more noise in change sets.

This ought to be factored into some common utils library contract, since the exact same computation needs to be computed in both L1 and L2.

Originally posted by @andrewgordstewart in #73 (comment)

What do you think about removing this restriction? It seems that the ticket signer can be different than the ticket sender as long as all book keeping is done with ticket signer as mapping keys.

Originally posted by @kerzhner in #1 (comment)

Is there a reason not to add /src/contract-types to the .gitignore on this project? Entire directory is generated (ie, non-source) files. They clog up the diff count!

I think that's a valid point (and the correct approach to follow according to typechain). I'll address this in a separate PR!

Originally posted by @lalexgap in #45 (comment)

Why is it is restriction that a receiver can only have one transfer in progress?

Originally posted by @kerzhner in #1 (comment)

Currently we don't consider timing for SAFE V2.

LGTM, but:

1. The linked page dates this recommendation from Dec 2019. Described rationale seems good to me, but two years may as well be 30 years.
2. This probably belongs in a separate PR

Originally posted by @NiloCK in #45 (comment)

The current prototype supports eth transfers. We'd prefer to do a head-to-head comparison against other protocols using ERC20 tokens, which is more representative of the real-world demand. In addition, the gas cost of sending eth is quite complicated: there are three ways of sending eth, and the often recommended way has a complex cost calculation.

There is little benefit in the prototype supporting multiple token types, so we will specialize the prototype to ERC20 tokens.

Create an initial solidity prototype of SAFE V2.

We should get an estimate of gas usage for our L2 contracts.

One thing to keep in mind is that L2 gas may be calculated differently than L1. So we may need to get a gas estimate using the OVM or AVM.

In advance of linking to it in a blog post

To allow Bob to satisfy L2 (https://www.notion.so/Desired-security-and-usability-properties-a07341b850274837ae48370e3e6b20e8?d=0884756d90e94183a876e574bf3d63ef#3924d381ae6f4ca68db1cee326366e5e) he needs a way to remove liquidity from L1.

@andrewgordstewart suggested Bob could just submit a ticket for himself. Does there need to be a special case for Bob so he doesn't have to lock funds up in escrow?

If we deploy to the optimism testnet we can easily get L2 gas costs for safe and compare them against computing them with a formula.

The current readme say to follow these instructions in the parent folder.

I think that would end up initialing a new yarn project in the parent folder which seems incorrect?

Update the prototype to support ERC20 tokens. This is partially complete here

See https://www.notion.so/statechannels/nxtp-adb1e5337a784216abc4b02e9a8fdddf

Note that since each swap involves the same two parties, this leads to an unrealistic benchmark. In the real world, we would expect many ERC20 transfers from one party (the LP) to many parties (users).

In the real world, there might be some repeated users, of course...

Originally posted by @andrewgordstewart in #93 (comment)

That's a good call. Contract fund redistribution needs to be refactored to:

• Factor out this logic.
• FIx the security concern
• Add erc20 support

Originally posted by @kerzhner in #73 (comment)

Yup, that's a good point!

We should be storing ticket commitments per sender and nonce. That way a sender can easily authorize many tickets at once.

Originally posted by @lalexgap in #1 (comment)

Currently I don't think solidity files are linted/formatted. We can probably base this on one of our previous repos.

1. Read through contracts of 2 products comparable to SAFE.
2. What features do these products support that SAFE does not? Are these essential features for a cross-chain product? If so, what is the difficulty level of adding these features to SAFE?

A more gas efficient method is to store a hash of the EscrowEntry

Originally posted by @kerzhner in #1 (comment)

The readme links to our Notion document. Our notion document isn't readible to persons outside of our organization.

Whatever PDF version is included in the submission should probably live in-repo and be the target of that readme link.

Currently our L2 and L1 contract use the same token contract.

In reality the token address will be different between L1 and L2, so we need a mechanism to map from one token address to another.

It makes sense to do this on L2 as gas costs will be much cheaper than L1.