-
Notifications
You must be signed in to change notification settings - Fork 0
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
Rename claim --> gift #46
Conversation
scripts/profile.ts
Outdated
const { txReceipt, gift, giftPrivateKey } = await defaultDepositTestSetup({ | ||
factory, | ||
useTxV3, | ||
overrides: { | ||
claimPrivateKey: 42n, | ||
giftTokenAddress: giftTokenContract.address, | ||
}, | ||
}); | ||
|
||
const { claim: claimExternalOj, claimPrivateKey: claimPrivateKeyExternal } = await defaultDepositTestSetup({ | ||
factory, | ||
useTxV3, | ||
overrides: { | ||
claimPrivateKey: 43n, |
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.
Removing this broke the profiler ^^'
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.
(You gotta make 2 gifts to be able to profile claim and claim external)
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.
oops
Co-authored-by: gaetbout <[email protected]>
God this is painful to review 🤣 |
/// @param factory The address of the factory | ||
/// @param class_hash The class hash of the gift account | ||
/// @param class_hash The class hash of the escrow account | ||
/// @param sender The address of the sender | ||
/// @param gift_token The ERC-20 token address of the gift |
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.
Unrelated but this sentence ins't clear to me
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.
Ignore it but let's keep it in mind later when we review doc
self: @TContractState, | ||
class_hash: ClassHash, | ||
escrow_class_hash: ClassHash, |
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.
to stay consistent, should we use the same naming as in deposit?
escrow_class_hash: ClassHash, | |
escrow_class_hash: ClassHash, |
if y, take care to update the doc plz
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.
i dont get it dont they have the same name?
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.
Wtf was my vscode doing '-'
You can close this remark x)
Banger branch name btw |
Claim --> gift
starknet_gifting -> argent_gifting
account --> escrow account
account impl --> library
didnt touch asserts yet!