Skip to content
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: Implement Calldata Decoding to JSC Types #1018

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

AryanGodara
Copy link

@AryanGodara AryanGodara commented Mar 15, 2024

Motivation and Resolution

Resolves #952

Currently involves calldata decoder, ie, calldata -> JSC types

Checklist:

  • Performed a self-review of the code
  • Rebased to the last commit of the target branch (or merged it into my branch)
  • Linked the issues which this PR resolves
  • Documented the changes in code (API docs will be generated automatically)
  • Updated the tests
  • All tests are passing

@AryanGodara AryanGodara marked this pull request as draft March 15, 2024 13:07
@AryanGodara AryanGodara changed the title WIP: Implementing Calldata Decoding to JSC Types feat: Implement Calldata Decoding to JSC Types Mar 15, 2024
@AryanGodara AryanGodara marked this pull request as ready for review March 15, 2024 19:43
@AryanGodara
Copy link
Author

@tabaktoni @ivpavici
I've tried to implement one-half of the issue and could really use your review on this one (no rush though, ik the weekend has started 😅)

@tabaktoni
Copy link
Collaborator

Good point splitting it into two subtasks is more manageable.
Will deal with it this week.

@AryanGodara
Copy link
Author

Good point splitting it into two subtasks is more manageable. Will deal with it this week.

Are the current changes a step in the right direction? If so, then I'll start to work on the API encoder part as well :D

src/utils/cairoDataTypes/uint256.ts Show resolved Hide resolved
src/utils/calldata/apiDataEncoder.ts Outdated Show resolved Hide resolved
src/utils/calldata/calldataDecoder.ts Outdated Show resolved Hide resolved
@tabaktoni
Copy link
Collaborator

In general, this is a good direction.
We need to have encoding < - > decoding tests for this.
So take some call data encode it and then decode, you should get back the initial objects sent to the calldata.

I think you would focus for now only on this part of the task as there are multiple data types to support this flow.
Some of them are extracted to singular classes like uin256 and decoding call data will be part of the class, but at the moment it is not so important as we can do it in future steps.

Priority is to have test type by type and mix types for this flow.

@AryanGodara
Copy link
Author

In general, this is a good direction. We need to have encoding < - > decoding tests for this. So take some call data encode it and then decode, you should get back the initial objects sent to the calldata.

I think you would focus for now only on this part of the task as there are multiple data types to support this flow. Some of them are extracted to singular classes like uin256 and decoding call data will be part of the class, but at the moment it is not so important as we can do it in future steps.

Priority is to have test type by type and mix types for this flow.

Hi, sorry I was busy last week and couldn't get to this task, I'll quickly get to addressing these, and implementing the rest of the solution :)

@AryanGodara
Copy link
Author

@tabaktoni I've written an initial test. Does this approach seem correct to you?
If so, I'll begin to write more testcases

describe('Encode-Decode CalldataField Flow', () => {

@tabaktoni
Copy link
Collaborator

tabaktoni commented Apr 23, 2024

Not sure what was the goal with Iterator, but in general I imagine test to be like:

import contractX....

const data = {
	list: [1, 3n], 
	balance: "0x34"
};

const cd = new CallData(contractX.abi);
const compiledCalldata = cd.compile('methodY', testData)
const decompiledCalldata = cd.decompile(compiledCalldata)

expect(data).toEqual(decompiledCalldata)

You can use any of predefined contracts abi we already have in mocks for example:

// wrong order
this can be data
and use

const contractCallData: CallData = new CallData(compiledComplexSierra.abi);

@AryanGodara AryanGodara changed the base branch from develop to next-version April 25, 2024 12:11
@PhilippeR26
Copy link
Collaborator

PhilippeR26 commented Apr 26, 2024

I am a bit in trouble to review this PR.
My DAPP for encoding/decoding is using Starknet.js v6.8.0, and do not needs any new functionality. So external tools have already all what is necessary to encode/decode calldata and custom types.
Is the purpose of this PR to have something more user friendly?

For example, if I want to decode the encoded parameters of a function, I just write 4 lines :

const encodedCalldata=["4674576345645645","2000","0"];
const fnName="transfer";

const myCallData=new CallData(abi);
const { inputs } = myCallData.parser.getLegacyFormat().find((abiItem:AbiEntry) => abiItem.name === fnName) as FunctionAbi;
const inputsTypes=inputs.map((inp:any)=>{return inp.type as string});
const result=myCallData.decodeParameters(inputsTypes,encodedCalldata);
console.log(result);

@AryanGodara
Copy link
Author

I am a bit in trouble to review this PR. My DAPP for encoding/decoding is using Starknet.js v6.8.0, and do not needs any new functionality. So external tools have already all what is necessary to encode/decode calldata and custom types. Is the purpose of this PR to have something more user friendly?

For example, if I want to decode the encoded parameters of a function, I just write 4 lines :

const encodedCalldata=["4674576345645645","2000","0"];
const fnName="transfer";

const myCallData=new CallData(abi);
const { inputs } = myCallData.parser.getLegacyFormat().find((abiItem:AbiEntry) => abiItem.name === fnName) as FunctionAbi;
const inputsTypes=inputs.map((inp:any)=>{return inp.type as string});
const result=myCallData.decodeParameters(inputsTypes,encodedCalldata);
console.log(result);

Hey, @PhilippeR26, Can you share a link for this code? I'd love to take a look as well.

As for the PR, from the issue, according to me, while you already have a great way to accomplish this, having a proper functionality setup within the repo itself would be both more user-friendly and also more sustainable. This will allow a seamless transition between the blockchain and JSC types without having to rely on external tools/write custom logic yourself for each new use case.

Plus, it improves flexibility and customization for handling complex data structures. Additionally, it'll help in validation, error handling, and testing, enhancing the reliability and maintainability of the repo.

@PhilippeR26
Copy link
Collaborator

https://github.com/PhilippeR26/starknet-encode-decode/blob/046970f58ff07ceb0cf852d4f1cf8638eee7db70/src/app/components/client/EncodeDecode/DecodeFunctionCalldata.tsx#L76-L78

Encoding of a custom type is working but isn't user friendly. It should be improved, to be as nice as decodeParameters.
image

@AryanGodara
Copy link
Author

https://github.com/PhilippeR26/starknet-encode-decode/blob/046970f58ff07ceb0cf852d4f1cf8638eee7db70/src/app/components/client/EncodeDecode/DecodeFunctionCalldata.tsx#L76-L78

Encoding of a custom type is working but isn't user friendly. It should be improved, to be as nice as decodeParameters. image

Thanks for this!
I'm working on improving the structure of the code I wrote above. It wasn't compatible with the existing format; so I'm rewriting it.

The PR isn't quite read for review at the moment, making lots of breaking changes, and I'll later squash all the temp commits into a final one.
So please feel free to not review for some time 😅

@PhilippeR26
Copy link
Collaborator

And if you want to encode a function response, you can use the current code this way :

const params = [cairo.tuple(300,new CairoOption<BigNumberish>(CairoOptionVariant.Some,50));]; 
const selectedType = "(core::felt252, core::option::Option::<core::integer::u8>)"; 
const iter = params[Symbol.iterator](); 
const structs = CallData.getAbiStruct(abi);
const enums = CallData.getAbiEnum(abi);
const abiExtract = abi.find((abiItem) => abiItem.name === selectedType); 
const inputAbi: AbiEntry = abiExtract ? 
    { name: abiExtract.type, type: abiExtract.name } :
    { name: "literal", type: responseType };
const result = parseCalldataField(iter, inputAbi, structs, enums);

Copy link
Collaborator

@tabaktoni tabaktoni left a comment

Choose a reason for hiding this comment

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

Some guidelines:

  • The response is only an object no need for an array.
  • Cleanup
  • re-enable lint src/
  • resolve circle dependency

@tabaktoni tabaktoni added the Type: feature New feature or request label May 3, 2024
@AryanGodara AryanGodara force-pushed the calldata-decoder-api-encoder branch from fb94876 to 764e316 Compare May 3, 2024 15:37
@AryanGodara AryanGodara requested a review from tabaktoni May 3, 2024 15:37
@penovicp penovicp deleted the branch starknet-io:develop June 21, 2024 04:26
@penovicp penovicp closed this Jun 21, 2024
@penovicp penovicp reopened this Jun 21, 2024
@penovicp penovicp changed the base branch from next-version to develop June 21, 2024 04:31
@ivpavici ivpavici requested review from tabaktoni and removed request for tabaktoni July 15, 2024 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Calldata decoder & API Encoder (with ABI)
5 participants