Skip to content

Conversation

@tzaffi
Copy link
Contributor

@tzaffi tzaffi commented May 12, 2023

Add missing field in EvalDelta

The Shared Resources work algorand/go-algorand#5035 introduced a new field in data.transactions.EvalDelta. This PR adds it to the mirror type types.EvalDelta.

Testing

As of 15May2023 there is a draft PR in go-algorand algorand/go-algorand#5381 with a test that validates the types against this PR.

@tzaffi tzaffi added the Bug-Fix label May 14, 2023
@tzaffi tzaffi marked this pull request as ready for review May 14, 2023 18:07
@tzaffi tzaffi changed the title adding EvalDelta.SharedAccts bugfix: adding EvalDelta.SharedAccts May 14, 2023
Copy link
Contributor

@algochoi algochoi left a comment

Choose a reason for hiding this comment

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

Looks good - it looks like the codec:allocbound is defined for all the fields here in go-algorand but not here (like logs, innerTxns). Is this inconsequential?

@tzaffi
Copy link
Contributor Author

tzaffi commented May 15, 2023

Looks good - it looks like the codec:allocbound is defined for all the fields here in go-algorand but not here (like logs, innerTxns). Is this inconsequential?

Good catch. I've modified the fields in EvalDelta to conform with other presentations of allocbound: EG

@Eric-Warehime - do you know if this is used programmatically in some way (you'd need reflection)? Or are these purely informational?

@Eric-Warehime
Copy link
Contributor

@Eric-Warehime - do you know if this is used programmatically in some way (you'd need reflection)? Or are these purely informational?

I think it's just used by the msgp gen code i.e. https://github.com/algorand/msgp/blob/26c81f33bddb2b3897a722290e67e9e363ca96d1/parse/getast.go#L423

So I don't think it's needed here--probably was just copied as is. But it's good to keep consistent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants