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

Consider using global variables instead of storage for some variables #15

Closed
DariusParvin opened this issue Jun 3, 2021 · 6 comments · Fixed by #53
Closed

Consider using global variables instead of storage for some variables #15

DariusParvin opened this issue Jun 3, 2021 · 6 comments · Fixed by #53
Assignees
Labels
enhancement New feature or request

Comments

@DariusParvin
Copy link
Contributor

DariusParvin commented Jun 3, 2021

From the SmartPy review #4:

Many of the fields in the storage (e.g. custFunding) do not change during the contract's lifetime. Using global variables or attributes (self.custFunding instead of self.data.custFunding) would lead to smaller storage and code.

For the variables which get set once at the beginning of the contract and never change, they can be hard coded in the contract, rather than being defined in the 'storage' part of the contract. It would make the contract smaller and therefore cost less to originate. It could be done for the global variables such as 'selfDelay', as well as channel specific variables such as cid, although doing it for the channel specific variables would add a lot of complexity to the establish flow.

The gains in efficiency would be relatively small (at least compared to a potential multichannel contract), so it's probably worth holding off on this until we explore that first.

The zkChannel spec is agnostic to exactly how the contract is specified. All that matters is that the merchant checks that what the customer originates on chain is what they expected given the same channel parameters.

@DariusParvin DariusParvin transferred this issue from boltlabs-inc/dialectic Jun 3, 2021
@DariusParvin DariusParvin added the enhancement New feature or request label Jun 3, 2021
@indomitableSwan
Copy link
Contributor

indomitableSwan commented Aug 24, 2021

@jakinyele Can you look at incorporating these changes? It seems like it may be relatively straightforward to implement these changes, and I'm unsure what complexity is added to the establish flow.

@DariusParvin
Copy link
Contributor Author

This issue is being addressed in the branch optimize-mercy-ps-to-global-storage. I am testing the changes on testnet to get updated benchmarks on the gas/storage costs. With the updated benchmarks, we'll be able to compare it to the other approach of storing just the hash of the merchant's PS public keys.

@indomitableSwan
Copy link
Contributor

Using global variables instead of storage for any channel-specific attributes introduces significant complexity in our code base, ie either the merchant or both parties will have to compile the contract using the SmartPy script. That is, if we switch merchant-specific attributes to global storage, then each merchant will have their own version of the contract, and if we switch customer-specific attributes as well, we would have a version of the contract for _each pair (or rather, channel).

Therefore, I suggest we do not consider adding channel-specific attributes to storage at this time. @DariusParvin can you follow up with a list of non-channel-specific attributes where it does/might make sense to consider this optimization?

@DariusParvin
Copy link
Contributor Author

DariusParvin commented Sep 2, 2021

The attributes that are possible to optimize are:
close_scalar, context_string, and self_delay.

I don't believe we have any intentions to make close_scalar or context_string customizable, in which case it would make sense for them to be global parameters.

I can think of two reasons we may want self_delay to not be fixed:

  • being able to vary the delay depending on how much tez is being held by the contract. For channels with larger amounts, the users may prefer to have a longer delay period.
  • it would make the contract more flexible for testing. The default value for self_delay according to the spec is 48hrs. If this were hard coded then it would take at least 48 hrs to test the custClaim and merchClaim entrypoints on testnet.

@indomitableSwan
Copy link
Contributor

The attributes that are possible to optimize are:
close_scalar, context_string, and self_delay.

I don't believe we have any intentions to make close_scalar or context_string customizable, in which case it would make sense for them to be global parameters.

I can think of two reasons we may want self_delay to not be fixed:

  • being able to vary the delay depending on how much tez is being held by the contract. For channels with larger amounts, the users may prefer to have a longer delay period.
  • it would make the contract more flexible for testing. The default value for self_delay according to the spec is 48hrs. If this were hard coded then it would take at least 48 hrs to test the custClaim and merchClaim entrypoints on testnet.

I agree with the above outlined approach. @jakinyele ?

@jakinyele
Copy link
Member

I also agree with the suggested approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants