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

Variable naming is confusing #40

Closed
Sliman4 opened this issue Sep 21, 2024 · 1 comment
Closed

Variable naming is confusing #40

Sliman4 opened this issue Sep 21, 2024 · 1 comment

Comments

@Sliman4
Copy link

Sliman4 commented Sep 21, 2024

For example, reconstructSignature:

const signature = transaction.addSignature(v, r, s);

Here the variable name is called signature. Even though, it contains a signed transaction.
Then, it returns the variable named signature:
return signature;

And the method itself is called reconstructSignature, so it would make sense for it to return a signature. But no!
const signedTransaction = await Eth.reconstructSignature(big_r, s, recovery_id, transaction, senderAddress);

Now it's signedTransaction when we use it in another file.

Second example:

const payload = transaction.getHashedMessageToSign();

The word payload is quite ambiguous by itself, one could argue that the serialized transaction that is sent to RPC is also a payload. So without much digging, it's easy to assume that here
const { transaction, payload } = await Eth.createPayload(senderAddress, contract, 0, data);
return { transaction, payload };

it's just a transaction that is serialized. Or something entirely different, because why does it return 2 objects if the second can be derived from the first in 1 method call. Not everyone knows that you can't sign a transaction, you have to sign its hash. But maybe there could at least be a comment so that people who never worked with signatures at low level can get a grasp of it quicker.

There are probably more ambiguous names, I didn't check the whole example, was just helping someone get started with chain signatures and they were following this example.

@gagdiez
Copy link
Collaborator

gagdiez commented Dec 3, 2024

changed names to a - hopefully - less confusing nomenclature, please open the issue again if the changes where not enough clear

@gagdiez gagdiez closed this as completed Dec 3, 2024
@github-project-automation github-project-automation bot moved this from NEW❗ to Shipped 🚀 in DevRel Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants