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/fetch cm from wallet and update cm #15

Merged
merged 45 commits into from
May 17, 2022

Conversation

aspnxdd
Copy link
Contributor

@aspnxdd aspnxdd commented May 10, 2022

This PR includes:

  • Fetch the Candy Machines accounts from the connected wallet
  • Retrieve the config on-chain from the connected wallet
  • Update some config from the candy machine on-chain
  • Transfer authority of the given candy machine to another wallet

Closes #9, #8

@begonaalvarezd begonaalvarezd changed the base branch from dev to feat/upload-cm May 10, 2022 16:11
storage: '',
files: [],
} as const;
function isFormUpdateIsValid(): boolean {
Copy link
Member

Choose a reason for hiding this comment

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

Double "is" is redundant, i think isFormUpdateValid is better

Comment on lines 12 to 15
Array.from(e.target.files).forEach((file) => {
fileList.push(file);
});
setCache(fileList[0]);
Copy link
Member

Choose a reason for hiding this comment

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

If only the first value of the array is necessary, why is the whole array iterated?


const { files, uploadAssets } = useUploadFiles();
const { cache, uploadCache } = useUploadCache();
const [interactingWithCM, setInteractingWithCM] = useState(false);
Copy link
Member

Choose a reason for hiding this comment

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

This state stores a boolean, i think it's better that the name starts with is isInteractingWithCM

solTreasuryAccount: PublicKey;
itemsRedeemed: BN;
}

export interface CandyMachineConfig {
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion! I was checking firefly best practices page, and they name the interfaces with a capital "i" at the beginning. In this case it would be ICandyMachineConfig
If you see it correct, we could implement it for all interfaces

https://iotaledger.github.io/firefly/guides/coding-conventions/naming/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I personally prefer not using prefixes or suffixes for types/interfaces

</Head>
<div className='flex justify-center items-center flex-col'>
<Title text='Update Candy Machine' />
<span className='mt-8'>{account}{" "}<a className='text-blue-700' href={`https://solscan.io/account/${account}?cluster=devnet`}>View in Solscan</a></span>
Copy link
Member

@cpl121 cpl121 May 12, 2022

Choose a reason for hiding this comment

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

I think its better that the link is opened in another tab

<a className='text-blue-700' href={"https://solscan.io/account/${account}?cluster=devnet"} target="_blank" rel="noopener noreferrer">View in Solscan</a>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally agree.

@@ -0,0 +1,84 @@
import { BN } from '@project-serum/anchor';

Copy link
Member

Choose a reason for hiding this comment

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

We can use moment.js to do many of the date functions

* @param time time to parse
* @returns {string} time parsed to UCT
*/
export function parseDateToUTC(dateTime: string, time: string): string {
Copy link
Member

Choose a reason for hiding this comment

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

Moment has a function for this moment.utc

return `${hours}:${minutes}`;
}

export function getCurrentTime() {
Copy link
Member

Choose a reason for hiding this comment

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

Moment has a function for this moment().format('LT')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are 2 reasons why I created these functions manually:

  1. Reduce size of the project, since we are only using 5 silly functions to parse dates, I decided to just write them up myself. We are not doing heavy date manipulation, therefore I consider it is not needed.
  2. All these functions (moment.utc() and moment().format('LT')) do not provide the desired outcome: if the date is "8:00", the desired outcome should be "08:00".
    According to the momentjs docs here the format("LT") provides "8:30 PM" but we would only need "08:30" (0 at the start and remove AM/PM).
    For the UTC we need "2 May 2021 08:00:00 GMT", but the moment.utc() provides "2013-02-04T18:35:24+00:00" for example.
    In this case I think we would not obtain any benefit by using moment.js

Copy link
Member

Choose a reason for hiding this comment

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

Nice!

const { connection } = useConnection();
const [candyMachineConfig, setCandyMachineConfig] =
useState<FetchedCandyMachineConfig>();
const [loading, setLoading] = useState({ loading: false, error: false });
Copy link
Member

Choose a reason for hiding this comment

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

I think this state checks if the loading spinner is shown, or an error has to be shown
If it does this, I think const [spinner, setSpinner] = useState({ loading: false, error: false }); better explains what this state does

View in Solscan
</a>
</span>
{loading.loading && <Spinner />}
Copy link
Member

Choose a reason for hiding this comment

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

I have commented the previous comment because this does not give me much information - loading.loading

CANDY_MACHINE_PROGRAM_V2_ID,
{
commitment: 'confirmed',

Copy link
Member

Choose a reason for hiding this comment

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

This space is not necessary

Base automatically changed from feat/upload-cm to dev May 12, 2022 13:37
Copy link
Member

@cpl121 cpl121 left a comment

Choose a reason for hiding this comment

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

Nice job!

@cpl121 cpl121 merged commit d990283 into dev May 17, 2022
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

Successfully merging this pull request may close these issues.

[Task]: Fetch candy machines from a wallet
2 participants