-
Notifications
You must be signed in to change notification settings - Fork 141
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
Publish and Claim: Inbox API for Capability Bootstrapping #1983
Conversation
Cadence Benchstat comparisonThis branch with compared with the base branch onflow:master commit 66b8591 Collapsed results for better readability
|
Codecov Report
@@ Coverage Diff @@
## master #1983 +/- ##
==========================================
- Coverage 77.44% 77.42% -0.03%
==========================================
Files 302 303 +1
Lines 62683 63128 +445
==========================================
+ Hits 48543 48874 +331
- Misses 12416 12510 +94
- Partials 1724 1744 +20
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
@@ -16486,6 +16486,8 @@ func (v *DictionaryValue) IsResourceKinded(interpreter *Interpreter) bool { | |||
type OptionalValue interface { | |||
Value | |||
isOptionalValue() | |||
iter(f func(Value)) | |||
fmap(inter *Interpreter, f func(Value) Value) OptionalValue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now we gotta extend optionals all the way with <*>
, >>=
, <|>
, and <>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would love to make OptionalValue
generic in its Value
(i.e. something like type OptionalValue[T any] interface { ...
so that less downcasting is necessary in cases like these, but it seems like a lot of restructuring of code in order to make Go not complain about circular references in the visitor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, what's currently missing compared to a full PR?
Consensus on the FLIP. This is explicitly a draft implementation of the FLIP as it stands currently. If the FLIP were accepted with no changes today, then this PR would be good-to-go as-is, but if the API changes in the FLIP then the PR will need to be updated to reflect that. |
Co-authored-by: Bastian Müller <bastian@axiomzen.co>
Co-authored-by: Bastian Müller <bastian@axiomzen.co>
…ving two saves per value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a couple storage-related issues left.
Also, the documentation checkbox in the description can be marked as done, the PR contains documentation for the new feature 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Only thing left is fixing the primitive static type constants
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
This is a rather large new feature, maybe hold off with merging this until we released the final version for the spork. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
nil, | ||
).(interpreter.AddressValue) | ||
|
||
publishedValue := interpreter.NewPublishedValue(inter, recipient, value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of transferring value
and recipient
separately, can we transfer the publishedValue
at once?
Although, I'm not sure what the Transfer
of PublishedValue
is supposed to do/be-like, maybe @turbolent knows better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, like noted above, Transfer
would have to handle its children, like e.g. SomeValue.Transfer
(or e.g. containers like Array
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just double-checking: was this added? couldn't find it in the newest commits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh; I was interpreted @turbolent's comment to mean that this would be handled in the case where we generalized PublishedValue
to be a container for arbitrary Value
types in the future. I can submit another PR with this fix if that wasn't the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Co-authored-by: Supun Setunga <supun.setunga@gmail.com>
…e into sainati/1951-publish-claim
Closes #1951
This is an implementation of the Publish/Claim API described in this FLIP: onflow/flow#1122
master
branchFiles changed
in the Github PR explorer