fix: fields fromstring not working as intended #7365
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
sklppy88
changed the title
sklppy88
force-pushed
the
ek/fix/7364/fields-fromstring-not-working-add-failing-test-case
branch
from
1340658 to
f7e0d07
Compare
Benchmark resultsMetrics with a significant change:
Detailed resultsAll benchmarks are run on txs on the This benchmark source data is available in JSON format on S3 here. Proof generationEach column represents the number of threads used in proof generation.
L2 block published to L1Each column represents the number of txs on an L2 block published to L1.
L2 chain processingEach column represents the number of blocks on the L2 chain where each block has 8 txs.
Circuits statsStats on running time and I/O sizes collected for every kernel circuit run across all benchmarks.
Stats on running time collected for app circuits
AVM SimulationTime to simulate various public functions in the AVM.
Public DB AccessTime to access various public DBs.
Tree insertion statsThe duration to insert a fixed batch of leaves into each tree type.
MiscellaneousTransaction sizes based on how many contract classes are registered in the tx.
Transaction size based on fee payment method | Metric | | |
| const arbitraryHexString = '0x123'; | ||
| const expectedBigInt = 291n; | ||
|
|
||
| expect(GrumpkinScalar.fromString(arbitraryString).toBigInt()).toEqual(expectedBigInt); |
There was a problem hiding this comment.
shouldn't arbitary string resolve to 123 and not 291?
There was a problem hiding this comment.
It's always resolved as hex currently, regardless of whether it's prefixed or not. Do we want to change the base behavior ?
There was a problem hiding this comment.
The base issue was that any string was getting truncated if it was an odd length.
There was a problem hiding this comment.
hmm can we make it clear then that fromString expects a hex string?
There was a problem hiding this comment.
Yep, added the comment as you specified in scrum, and refactored the check because the check didn't actually work for a hexstring like 12xx34xx56.
There was a problem hiding this comment.
@sklppy88 can this problem get revised? I am hitting this issue in a deep object, so i cant easily convert all my strings to hex format
There was a problem hiding this comment.
Hello @olehmisar: I haven't had the green light to change the current behavior, but will your problem be alleviated if I add a param for you to specify the encoding ?
There was a problem hiding this comment.
See here, would this fix your problem ?
There was a problem hiding this comment.
@sklppy88 i am have to do something like this:
encodeArguments(abi, args.map((arg) => {
if (Number.isFinite(Number(arg))) {
return new Fr(BigInt(arg));
}
return arg;
}));Now imagine that args contains a nested object or array. i will have to iterate over each object deeply (prob using recursion) to convert each number into a hex string manually.
Your example won't help as it will just change:
- return new Fr(BigInt(arg));
+ return new Fr(arg, "dec");But if you fixed the original problem, i would be able to just write this code:
encodeArguments(abi, args)There was a problem hiding this comment.
could you please explain the reasoning behind not wanting to change it to autodetection of decimal vs hex? For me it's such an obvious decision. What am i missing?
sklppy88
force-pushed
the
ek/fix/7364/fields-fromstring-not-working-add-failing-test-case
branch
from
f7e0d07 to
2ec5610
Compare
sklppy88
force-pushed
the
ek/fix/7364/fields-fromstring-not-working-add-failing-test-case
branch
from
2ec5610 to
337bd61
Compare
Please read [contributing guidelines](CONTRIBUTING.md) and remove this line.
sklppy88
changed the title
sklppy88
changed the title
sklppy88
deleted the
ek/fix/7364/fields-fromstring-not-working-add-failing-test-case
branch
Please read contributing guidelines and remove this line.