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

(WIP) refactor(promise)!: refactor promises to schedule at end of function call and allow automatically assigning gas #523

Closed
wants to merge 32 commits into from

Conversation

austinabell
Copy link
Contributor

@austinabell austinabell commented Aug 9, 2021

PoC of ideas brought up on zulip, things are very rough and will stay this way until agreed this is the direction, but opening for visibility and if anyone has any suggestions.

  • refactors away promise drop pattern
  • Keeps all batch promises in a queue and adds a function call in codegen to schedule these queued promises
  • ext_contract codegen now returns a new type, which acts as a builder pattern

Open questions:

  • Include env::promise API in this queued pattern? Seems like a footgun to me, but maybe no more than the original idea
  • Can't actually estimate gas correctly because even if you know how much gas remaining, you don't know how much gas the scheduling and remaining logic will use to assign prepaid gas correctly. The gas estimation is super naive right now, it just splits the prepaid gas equally among function calls (so same logic as the ones replaced)
  • Promise API can be changed to not move all Promise types now that drop isn't relied on
  • Should this new FunctionCallBuilder type be in this pseudo hidden module so it's not used otherwise?
  • Calling as_return can schedule promise_return twice (done automatically on serialize), more logic is required to handle this case

gas: Option<Gas>,
}

impl FunctionCallBuilder {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest we don't introduce new structure FunctionCallBuilder and just implement these methods to Promise class itself. This will avoid having into_promise method that all users need to call, which would make API simpler.

AFAIK there are two ways the builder pattern can be implemented:

  • With one struct/class. FinalStruct that has methods add_field1(FinalStruct,...) -> FinalStruct;
  • With two structs/classes. Builder that has methods add_field1(Builder,...) -> Builder and finalize(Builder)->FinalStruct.

AFAIU the second one is typically used when FinalStruct is heavy (e.g. building an RPC server), so I think we should go with the first one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would require having with_gas and with_balance as part of the Promise API, when you would not be able to know which part of the promise you were specifying gas/balance for. There needs to be some compile-time restriction to scope this function call builder from the Promise, else this becomes a really janky API IMO.

Because you can't Deref into an owned type and because the promise builder pattern is <function>(self,.. ) -> Self instead of references to self, this is why the into_promise is required.

@austinabell
Copy link
Contributor Author

Closing in favour of #742.

@austinabell austinabell closed this Mar 4, 2022
@austinabell austinabell deleted the austin/tmp/queue_fc branch September 20, 2022 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants