enhance ToGoValue() in cadence.Value #2531

bjartek · 2023-06-05T10:47:57Z

Description

Started on suggesting new ToGo() values

Targeted PR against master branch
Linked to Github issue with discussion and accepted design OR link to spec that describes this work
Code follows the standards mentioned here
Updated relevant documentation
Re-reviewed Files changed in the Github PR explorer
Added appropriate labels

…tuall dictionary

turbolent · 2023-06-06T00:51:42Z

values.go

-	return string(v)
+	value := string(v)
+	if value == "" {
+		return nil


Why return nil here for an empty string?

I guess this is just a leftover from my blindly porting over the overflow code. The reasoning there was to make the output as terse as possible.

I can change this all the places here because I think that in cadence we should not make this distinction.

turbolent · 2023-06-06T00:52:23Z

values.go

@@ -333,7 +338,7 @@ func (Address) MeteredType(common.MemoryGauge) Type {
 }

 func (v Address) ToGoValue() any {
-	return [AddressLength]byte(v)
+	return v.String()


Why change to a string here? A Go byte array seems like a better mapping

The reason i did this is that i just find 0x123 easier to work with and reason about then a byte array, but that might just be me.

turbolent · 2023-06-06T00:53:47Z

values.go

+	if err != nil {
+		panic(err)
+	}
+	return value


Fixed point numbers are not floating point numbers. This potentially a lossy conversion, as some fixed-point values cannot be represented as floating point numbers.

Interesting, what situations would that occur in?

turbolent · 2023-06-06T00:54:36Z

values.go

+	if len(ret) == 0 {
+		return nil
+	}


Maybe move this to the top of the function, before unnecessarily allocating an empty slice

If we want to skip nil values in the list we cant do this. Do we want to do that or not? I personally prefer it, but if you do not want it I can change it around.

turbolent · 2023-06-06T00:56:29Z

values.go

@@ -1617,12 +1636,18 @@ func (v Dictionary) WithType(dictionaryType *DictionaryType) Dictionary {
 }

 func (v Dictionary) ToGoValue() any {
-	ret := map[any]any{}
+	ret := map[string]any{}


Why change the key type to always being a string? What if the key type of the dictionary is e.g. Int?

Another thing left over from overflow. Will fix.

turbolent · 2023-06-06T00:59:20Z

values_test.go

 			value: NewArray([]Value{
-				NewInt(10),


Instead of changing the existing value, add an additional one

The old test has an array that has both and int and a string, I can always add it back i just though it was very strange.

bjartek · 2023-06-15T08:22:38Z

values.go

@@ -1493,7 +1502,11 @@ func (v UFix64) MeteredType(common.MemoryGauge) Type {
 }

 func (v UFix64) ToGoValue() any {
-	return uint64(v)
+	value, err := strconv.ParseFloat(v.String(), 64)


But here it is OK to use ParseFloat or can we not do that as per the comment in Fix64?

bjartek · 2023-06-15T08:39:55Z

Sorry for the delay here, I added some comments and fixed some things!

bjartek · 2023-08-21T10:58:19Z

Status?

turbolent · 2023-08-22T17:49:20Z

@SupunS Could you please own this?

SupunS · 2023-08-22T17:55:16Z

Yeah, can do. I'll have a look later today.

In the meantime, were there any particular use-case(s) we wanted to enhance, or is this just a general improvement?

SupunS · 2023-09-05T20:24:17Z

Discussion around the use-cases: https://discord.com/channels/613813861610684416/1108479699732152503/1146911629481619606

turbolent · 2024-04-24T16:58:07Z

Given there are many different use cases and needs, and there is no "correct" Go type to return in all cases.
Developers might expect a certain Go type to be returned, but the return type is any, so it makes the API vry brittle.

It might be a good idea to "remove" this method from Value, and instead:

Have developers define/use their own
Provide multiple implementations of the method as stand-alone functions. For example, we already have well-defined conversion functions, like ByteArrayValueToByteSlice ("Cadence byte array [UInt8] to Go byte slice []byte").
We could extract/refactor the existing methods in a similar way, and have this PR add additional conversion functions.

How does that sound @bjartek?

turbolent · 2024-05-01T16:19:55Z

ToGoValue got removed in #3291. As mentioned above, maybe the removed functions and the new variants of the functions in this PR can be converted to co-existing global functions that convert to concrete return types (instead of any with ToGoValue).

bjartek · 2024-05-01T16:21:26Z

I dont have much time for this atm :( a lot to do.

turbolent · 2024-05-01T16:52:26Z

I dont have much time for this atm :( a lot to do.

No worries! We can pick this up after shipping Cadence 1.0, thank you for these contributions and your patience with getting them in

bjartek · 2024-05-01T17:10:05Z

When will the removal of ToGoValue() hit a network? I think i use that in overflow/underflow.

turbolent · 2024-05-01T17:12:20Z

When will the removal of ToGoValue() hit a network?

It's not affecting networks, only code that imports Cadence / the cadence package. So basically, as soon as that code updates to the latest version.

bjartek · 2024-05-02T09:05:33Z

Why was removing ToGoValue() not a flip? this is a pretty breaking change.

turbolent · 2024-05-02T16:05:12Z

@bjartek We only have FLIPs for the language, not the implementation. The fact that we expose some functionality from this implementation of the language is just a side-effect, there are no guarantees about what is exposed from it.

In any case, the functionality got removed from the package, but it can be easily provided by third-party code (e.g. another open-source Go module, or directly as part of an application), none of the underlying functionality needed for it is gone.

bjartek · 2024-05-02T16:28:30Z

Can I ask why it is removed? Ref Hyrums law. The cost og this change could be quite high.

turbolent · 2024-05-02T16:39:50Z

See my comment above: #2531 (comment). I had also tried to explain it here: onflow/flow-go#5820 (comment)

The goal of #3290 was to remove access by index. Value.ToGoValue for composites (structs, resources, etc.) still returned field values as a slice, which basically left a "loop-hole", where users could keep making wrong assumptions (order of fields). Changing the result of ToGoValue for composites, e.g. to return a map, would have silently (!) broken all users of the function, because they likely expected (used a cast to) a slice, e.g. Struct.ToGoValue.([]cadence.Value).

The removal was literally a result of this PR: We cannot change the implementations of ToGoValue, because downstream, users actually depend on a particular result type.

bjartek · 2024-05-02T16:49:50Z

Ok. So when will this version of cadence be used? Because this is more work for everybody and they need to know that they have to do the work.

turbolent · 2024-05-02T16:55:12Z

@bjartek I though I had already answered the same question above: #2531 (comment).

This change only affects developers using the Go package, and the changes are in the release notes.

bjartek · 2024-05-08T09:41:51Z

So i assume this can just be closed then?

turbolent · 2024-05-08T16:51:36Z

@bjartek yeah, I guess we can close this, given in it's current form it won't apply. We can definitely add such conversion functionality back to the code, but ideally in a well-defined (so it matches e.g. JS SDK) and strongly typed fashion (no any return type).

bjartek added 2 commits June 5, 2023 12:45

started on this, still missing struct/resource to not be array but ac…

ba47316

…tuall dictionary

added more example to go values

c70152d

bjartek changed the title ~~started on this, still missing struct/resource to not be array but ac…~~ enhance ToGoValue() in cadence.Value Jun 5, 2023

bjartek marked this pull request as ready for review June 5, 2023 19:26

bjartek requested review from turbolent, SupunS and dsainati1 as code owners June 5, 2023 19:26

turbolent reviewed Jun 6, 2023

View reviewed changes

turbolent self-assigned this Jun 6, 2023

turbolent added the Improvement label Jun 6, 2023

turbolent reviewed Jun 6, 2023

View reviewed changes

bjartek commented Jun 15, 2023

View reviewed changes

bjartek added 3 commits June 15, 2023 10:29

replace nil with empty string for ToGoValue of string

a808bc9

fixed type of map to be any

2d88ed3

fixing the array test, adding back the old one

a957007

turbolent added the Go API Breaking Change Breaks programs which use the Cadence repo as a Go dependency label Jun 28, 2023

turbolent assigned SupunS and unassigned turbolent Aug 22, 2023

This was referenced Apr 26, 2024

Remove Value.ToGoValue and NewValue #3291

Merged

Update to Cadence v1.0.0-preview.24 onflow/flow-go#5820

Merged

turbolent closed this May 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enhance ToGoValue() in cadence.Value #2531

enhance ToGoValue() in cadence.Value #2531

bjartek commented Jun 5, 2023 •

edited by turbolent

Loading

turbolent Jun 6, 2023

bjartek Jun 15, 2023

turbolent Jun 6, 2023

bjartek Jun 15, 2023 •

edited

Loading

turbolent Jun 6, 2023

bjartek Jun 15, 2023

turbolent Jun 6, 2023

bjartek Jun 15, 2023

turbolent Jun 6, 2023

bjartek Jun 15, 2023

turbolent Jun 6, 2023

bjartek Jun 15, 2023

bjartek Jun 15, 2023

bjartek commented Jun 15, 2023

bjartek commented Aug 21, 2023

turbolent commented Aug 22, 2023

SupunS commented Aug 22, 2023

SupunS commented Sep 5, 2023

turbolent commented Apr 24, 2024

turbolent commented May 1, 2024

bjartek commented May 1, 2024

turbolent commented May 1, 2024

bjartek commented May 1, 2024

turbolent commented May 1, 2024

bjartek commented May 2, 2024

turbolent commented May 2, 2024

bjartek commented May 2, 2024

turbolent commented May 2, 2024 •

edited

Loading

bjartek commented May 2, 2024

turbolent commented May 2, 2024

bjartek commented May 8, 2024

turbolent commented May 8, 2024

enhance ToGoValue() in cadence.Value #2531

enhance ToGoValue() in cadence.Value #2531

Conversation

bjartek commented Jun 5, 2023 • edited by turbolent Loading

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjartek Jun 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjartek commented Jun 15, 2023

bjartek commented Aug 21, 2023

turbolent commented Aug 22, 2023

SupunS commented Aug 22, 2023

SupunS commented Sep 5, 2023

turbolent commented Apr 24, 2024

turbolent commented May 1, 2024

bjartek commented May 1, 2024

turbolent commented May 1, 2024

bjartek commented May 1, 2024

turbolent commented May 1, 2024

bjartek commented May 2, 2024

turbolent commented May 2, 2024

bjartek commented May 2, 2024

turbolent commented May 2, 2024 • edited Loading

bjartek commented May 2, 2024

turbolent commented May 2, 2024

bjartek commented May 8, 2024

turbolent commented May 8, 2024

bjartek commented Jun 5, 2023 •

edited by turbolent

Loading

bjartek Jun 15, 2023 •

edited

Loading

turbolent commented May 2, 2024 •

edited

Loading