QPQ-AG/sophia
10
0
Fork 0
Code Issues 41 Pull Requests 5 Actions Packages Projects Releases 16 Wiki Activity

Pt 166636909 avoid calldata collision #578

Merged
zxq9 merged 4 commits from PT-166636909-avoid-calldata-collision into master 2019-06-14 20:09:25 +09:00
Conversation 22 Commits 4 Files Changed 5 +153 -35
zxq9 commented 2019-06-12 23:36:41 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: ThomasArts

*Created by: ThomasArts*
zxq9 commented 2019-06-13 02:39:15 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: hanssv

This will only work for nulllary functions (since the added function will always have 0 arguments...) This is a problem in the original PR not introduced here.

*Created by: hanssv* This will only work for nulllary functions (since the added function will always have 0 arguments...) This is a problem in the original PR not introduced here.
zxq9 commented 2019-06-13 02:40:10 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: hanssv

We should have a test that actually creates calldata for something with arguments :-)

*Created by: hanssv* We should have a test that actually creates calldata for something with arguments :-)
zxq9 commented 2019-06-13 19:02:33 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: ThomasArts

Yes, done that now

*Created by: ThomasArts* Yes, done that now
zxq9 commented 2019-06-13 19:45:14 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: UlfNorell

con is not list cons, but variant type construction (Ar is the list of arities of the constructors, I is the tag index, and As is the constructor arguments). Cons is {op, '::', [Hd, Tl]}.

*Created by: UlfNorell* `con` is not list cons, but variant type construction (`Ar` is the list of arities of the constructors, `I` is the tag index, and `As` is the constructor arguments). Cons is `{op, '::', [Hd, Tl]}`.
zxq9 commented 2019-06-13 19:52:32 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: UlfNorell

TODO:

  • variant types
  • negative integers ({op, '-', [{lit, {int, N}}]})
  • Bits (not working for aevm though)
  • hash and signature literals (coordinate with @hanssv)
*Created by: UlfNorell* TODO: - variant types - negative integers (`{op, '-', [{lit, {int, N}}]}`) - Bits (not working for aevm though) - hash and signature literals (coordinate with @hanssv)
zxq9 commented 2019-06-13 19:53:53 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: UlfNorell

Or better: break out lit_to_fate from term_to_fate and use that.

*Created by: UlfNorell* Or better: break out `lit_to_fate` from `term_to_fate` and use that.
zxq9 commented 2019-06-13 19:56:24 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: hanssv

Where?

*Created by: hanssv* Where?
zxq9 commented 2019-06-13 19:56:44 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: UlfNorell

I don't see the tests. You mean done, but not pushed?

*Created by: UlfNorell* I don't see the tests. You mean done, but not pushed?
zxq9 commented 2019-06-14 15:47:36 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: ThomasArts

Thanks. Will check with @hanssv. Pushed the test for dutch_auction, counter and bitcoin_auth that take different arguments. Bitcoin_auth is not compilable yet, but when it is, test is ready.

*Created by: ThomasArts* Thanks. Will check with @hanssv. Pushed the test for dutch_auction, counter and bitcoin_auth that take different arguments. Bitcoin_auth is not compilable yet, but when it is, test is ready.
zxq9 commented 2019-06-14 17:54:32 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: UlfNorell

The test would look nicer if you defined a single contract with functions whose arguments cover all the cases in term_to_fate. Choosing a different existing test contract and defining a new unit test for each case is a bit clumsy, and it's hard to get an overview of the test coverage.

*Created by: UlfNorell* The test would look nicer if you defined a single contract with functions whose arguments cover all the cases in `term_to_fate`. Choosing a different existing test contract and defining a new unit test for each case is a bit clumsy, and it's hard to get an overview of the test coverage.
zxq9 commented 2019-06-14 18:03:51 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: UlfNorell

term_to_fate({op, '::', [Hd, Tl]}) ->
*Created by: UlfNorell* ```suggestion:-0+0 term_to_fate({op, '::', [Hd, Tl]}) -> ```
zxq9 commented 2019-06-14 18:04:20 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: UlfNorell

    aeb_fate_data:make_list([term_to_fate(Hd) | term_to_fate(Tl)]);
*Created by: UlfNorell* ```suggestion:-0+0 aeb_fate_data:make_list([term_to_fate(Hd) | term_to_fate(Tl)]); ```
zxq9 commented 2019-06-14 18:06:36 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: UlfNorell

Works, since make_list is identity. Should really do ?FATE_LIST_VALUE on the tail.

*Created by: UlfNorell* Works, since `make_list` is identity. Should really do `?FATE_LIST_VALUE` on the tail.
zxq9 commented 2019-06-14 19:05:15 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: UlfNorell

The second make_list makes no sense. term_to_fate already adds a make_list so if anything that make_list should be stripped off (but make_list is currently a no-op, so...). I guess the most correct thing to do without messing with aebytecode (and adding aeb_fate_data:X_value functions) is to include aeb_fate_data.hrl and use ?FATE_LIST_VALUE.

*Created by: UlfNorell* The second `make_list` makes no sense. `term_to_fate` already adds a `make_list` so if anything that `make_list` should be stripped off (but `make_list` is currently a no-op, so...). I guess the most correct thing to do without messing with aebytecode (and adding `aeb_fate_data:X_value` functions) is to include `aeb_fate_data.hrl` and use `?FATE_LIST_VALUE`.
zxq9 commented 2019-06-14 19:11:11 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: ThomasArts

In that case I probably would just only use term_to_fate(Tl) because including that abstraction breaker in this code seems weird.

*Created by: ThomasArts* In that case I probably would just only use `term_to_fate(Tl)` because including that abstraction breaker in this code seems weird.
zxq9 commented 2019-06-14 19:11:34 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: ThomasArts

I can do a comment on why it works :-).

*Created by: ThomasArts* I can do a comment on why it works :-).
zxq9 commented 2019-06-14 19:12:49 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: hanssv

Signatures and hash are missing, but it is probably easier if I take care of them in a follow up PR.

*Created by: hanssv* Signatures and hash are missing, but it is probably easier if I take care of them in a follow up PR.
zxq9 commented 2019-06-14 19:14:28 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: UlfNorell

Remove the outer make_list as well then.

*Created by: UlfNorell* Remove the outer `make_list` as well then.
zxq9 commented 2019-06-14 19:50:09 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: UlfNorell

also missing: maps

*Created by: UlfNorell* also missing: maps
zxq9 commented 2019-06-14 19:51:30 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: UlfNorell

Review: Approved

*Created by: UlfNorell* **Review:** Approved
zxq9 commented 2019-06-14 20:08:49 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: hanssv

Review: Approved

*Created by: hanssv* **Review:** Approved
zxq9 commented 2019-06-14 20:09:25 +09:00 (Migrated from gitlab.com)
Copy Link

Merged by: ThomasArts at 2019-06-14 11:09:25 UTC

*Merged by: ThomasArts at 2019-06-14 11:09:25 UTC*
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
WIP
bug
consensus-breaking
dependencies
documentation
duplicate
effort: high
effort: low
effort: medium
effort: trivial
enhancement
good first issue
help wanted
invalid
maintenance
question
task/feature
todo-in-rewrite
wontfix
bug
Something is not working
duplicate
This issue or pull request already exists
enhancement
New feature
help wanted
Need some help
invalid
Something is wrong
pig lipstick
Muggle-facing enhancements
question
More information is needed
wontfix
This won't be fixed
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: QPQ-AG/sophia#578
No description provided.
Delete Branch "PT-166636909-avoid-calldata-collision"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?