-
Notifications
You must be signed in to change notification settings - Fork 114
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
feat: new functions in stdlib from stdlib.fc
and math.fc
#986
base: main
Are you sure you want to change the base?
Conversation
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.
Sorry for reviewing the draft, I just wanted to keep those notes around :)
178e712
to
b9b6206
Compare
@novusnota @anton-trunov Implemented all functions. I'd like to hear some intermediate feedback on them before I proceed with writing tests. I made sure to include all missing functions from both stdlib.fc and mathlib.fc with some exceptions (see PR description). |
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.
afair, mathlib.fc
has lots of functions for fixed-arithmetic as well, I think we should add those too (perhaps as a separate library, like fixed_point_math.tact
).
Also, I left a couple comments in the source code
stdlib/std/cells.tact
Outdated
refs: Int; | ||
} | ||
|
||
asm fun computeDataSize(cell: Cell, maxCells: Int): DataSize { CDATASIZE TRIPLE } |
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.
Would TRIPLE
work here? Structures are returned as tensors, right?
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.
TRIPLE
makes three elements into a tuple of length 3. I think it should work
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, but our structs are tensors, not tuples, right?
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.
structs are implemented as tuples in Tact (that's why we had problems with long ones in the first place, remember?)
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.
ah, I must have mistaken it with the unbundled approach in the storage
src/generator/writers/writeStdlib.ts
Outdated
ctx.asm("", "UBITSIZE"); | ||
}); | ||
|
||
ctx.fun(`__tact_sqrt`, () => { |
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.
how about we implement this in Tact?
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.
and what about other stdlib functions?
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.
if a function can be implemented efficiently in Tact, we should implement it in Tact or open an issue and fix it
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.
done
@anton-trunov @novusnota covered all new functions with tests! Found and fixed a few little mistakes in types (for |
@Gusarich Let's
|
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.
We should document the new functions in https://docs.tact-lang.org/ref/core-math and other relevant doc pages
Will do |
Co-authored-by: Novus Nota <[email protected]>
8a5fed9
to
734f3a0
Compare
@anton-trunov rebased |
} | ||
|
||
get fun addressNone(): Address? { |
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.
Why does it return "Maybe Address" and not "Address" ?
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.
Because the way Tact currently work with addresses is that "null" as an address is actually addressNone
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.
Well, I checked the sources and you are 100% right, but maybe we should change it?
Or change the tlb schemes generation, because
struct Test {
my_addr: Address?;
}
results
## Test
TLB: `_ my_addr:Maybe address = Test`
Signature: `Test{my_addr:Maybe address}`
In .md file (and maybe in some other places)
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.
it's an old problem in Tact, which was blocking multiple issues in the past. I remember some old comments in address-related issues about that.
My opinion is that we should rework addresses, because currently the way they work is not intuitive and doesn't fit into how other types work.
Basically, addr_none
should be a valid part of Address
type and should be checked by devs against addressNone()
(and not null
). And Address?
should mean Maybe Address
, which also could contain addr_none
.
@anton-trunov what do you think?
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.
addr_none should be a valid part of Address type and should be checked by devs against addressNone() (and not null).
yeah, this is how it should work, indeed
And Address? should mean Maybe Address
also agreed
which also could contain addr_none.
this one I'm not sure about, I'd say addr_none in the optional version should be 000
, what do you think?
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.
this one I'm not sure about, I'd say addr_none in the optional version should be
000
, what do you think?
well yes, that's what I meant. I was talking about the possibility of the case when value of Address?
is equal to addressNone()
.
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.
@Gusarich Maybe you could take care of this change first (allowing addr_none
as an inhabitant of the Address
type) and then we merge this PR with the corresponding changes
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.
We could also probably add a config option to turn off address verification along the way
Halfway there, will be finishing this soon. UPD: 2/3rds are done, got to complete the cell related functions |
… documented, except for `addressNone`
@anton-trunov have a look at the updated notes. there are few of them that I don't think we need to cover in Tact |
Looks good! Thanks for adding those |
Added documentation for all of the new functions from this PR, except for the |
Issue
Closes #770, closes #1029
Checklist
docs/
and made the build locallyCurrent State
minmax
and noslice_bits_refs
because we don't have tensor types in Tact and it would be useless: Tensor types #1116set_data
andget_data
because Tact manages persistent storage by itselfbless
and other continuation functions because Tact manages control flow itself and for running some arbitrary code it's better to use RUNVM which doesn't require continuations parametersraw_reserve_extra
because it requires anuint32 -> varuint32
dictionary and we only supportvaruint16
for values currently:VarUInt32
,VarInt16
,VarInt32
serialization types #1117nan
andis_nan
because we don't support NaN in Tact. Do we really need them? @anton-trunovsub_rev
because it is useless in our case...MOD
instructions because they return two values as a tensor and it won't be very developer-friendly in Tact to make them return tuples in my opinion: Tensor types #1116